Skip to content

Conversation

@Batta32
Copy link
Contributor

@Batta32 Batta32 commented Feb 25, 2021

Description

The SpanishDateTimeParserConfiguration doesn't recognize the SpanishDateTime.LastNightTimeRegex as the resource is not updated. The compilation fails without updating the resource.

Specific Changes

Update the SpanishDateTime resource with the latest changes.

Testing

Java build correctly passing
image

Copy link
Collaborator

@tellarin tellarin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the proper way to handle resource updates. The build process should automatically update such resource code files during the build process (via the YAML files).
Perhaps there is a silent failure somewhere in the resource generation process?
Downstream consumers of the recognizers should rely on the generated packages and not in code files in the repository, if that's the case here.

@Batta32
Copy link
Contributor Author

Batta32 commented Feb 26, 2021

Hi @tellarin, these changes were added after executing the build.cmd of Java, they are not manually updated.

The compilation of datetime is failing as the property LastNightTimeRegex is not present in SpanishDateTime and it's already used in SpanishDateTimeParserConfiguration. However, the build.cmd won't fail because, as you mentioned, these changes are generated but they are not present in the repository.

To reproduce the issue using master branch:

  1. Open Java Recognizers-Text
  2. Build recognizers-text-date-time library
  3. See the compilation error

Compilation error in Datetime recognizers of Java
image

@tellarin
Copy link
Collaborator

tellarin commented Mar 4, 2021

@Batta32, I meant to say that if the build process generates such files correctly, than there can never be a compilation failure.
The correct fix is to try to identify if the resource code is not generated at the right time and fix the build process.
Perhaps the resource gen step is happening after the relevant project is being built?
Adding the generated code file would just be a mitigation that would hide the root problem until the next YAML update.

@Batta32
Copy link
Contributor Author

Batta32 commented Mar 4, 2021

@tellarin - doing the manual build (executing mvn clean install) will fail because of this. As you said, the build.cmd will generate such files correctly as they are consuming the updated YAMLs.

C# is working as PR #2503 introduced the changes in the DateTimeDefinitions, however Java/JavaScript/Python don't contain those last changes in the autogenerated resources.

Just to know, which should be the correct way to update the autogenerated files in order to have the mvn clean install working?

@tellarin
Copy link
Collaborator

tellarin commented Mar 5, 2021

@Batta32, think we're talking past each other.
The original design is for the build process to auto-re-generate any resource code from YAMLs, ideally, if the YAML is newer than the corresponding platform-specific code (.NET/JS/Java/Python) in the repo.
One reason for this is exactly to avoid such problems as you are seeing, as many times contributors change the YAML file and re-gen the code in one platform that they are using to test, but not in the others.
The CI build process is supposed to catch if such new changes cause build issues, so it needs to generate the necessary code updates to make sure no new changes are missing.
.NET implements it like this (only re-gen from YAML if it's newer than the corresponding existing .cs code), while JS and Python re-gen the resources on every build.
If build.cmd for Java re-gens the resources on every build and the build doesn't fail, couldn't "mvn clean install" do the same Otherwise the build process will never validate that "mvn clean install" will work correctly.
That's what I mean when I say that adding the file may mitigate the break you mention, but is not solving the root issue and it will likely break again as soon as someone updates some YAML file.

@Batta32
Copy link
Contributor Author

Batta32 commented Mar 5, 2021

Awesome @tellarin! Thanks for the explanation. I'll close this PR then 😊.

@Batta32 Batta32 closed this Mar 5, 2021
@Batta32 Batta32 deleted the feature/southworks/java/datetime-update-resources branch April 9, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants