Skip to content

Fix aviation empty entry and formatting. #762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 12, 2023
Merged

Fix aviation empty entry and formatting. #762

merged 2 commits into from
Apr 12, 2023

Conversation

bodiam
Copy link
Contributor

@bodiam bodiam commented Apr 12, 2023

No description provided.

@bodiam bodiam requested a review from panilya April 12, 2023 14:19
Copy link
Collaborator

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

Should we be using/mandating quotes more specifically?

Is there something we should be adding to tests here? (Not specific to aviation, but in general)

Copy link
Collaborator

@panilya panilya left a comment

Choose a reason for hiding this comment

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

LGTM!

@bodiam bodiam merged commit 544c3f0 into main Apr 12, 2023
@bodiam
Copy link
Contributor Author

bodiam commented Apr 12, 2023

Should we be using/mandating quotes more specifically?

With what purpose?

Is there something we should be adding to tests here? (Not specific to aviation, but in general)

Maybe. I think it would be good to make sure a result can never be an empty string or so? (I assume that would have been the result of this?)

@kingthorin
Copy link
Collaborator

  1. Just for consistency. I have no idea why half of these have quotes and half don’t. As a new contributor it would/could seem messy 🤷‍♂️
  2. I’ll see if I can add a not empty condition to the test set we abstracted a few months ago.

@kingthorin kingthorin deleted the fix_aviation branch April 12, 2023 21:27
@bodiam
Copy link
Contributor Author

bodiam commented Apr 12, 2023

Regarding 1, I had the same. I went through a few yml files to check them, but it's very inconsistent, but changing this might be a bit of an effort.

Regarding 2: thanks!

@kingthorin
Copy link
Collaborator

Looks like we could make it consistent leveraging spotless https://github.com/diffplug/spotless/tree/main/plugin-maven#yaml

We already use spotless so shall I look into doing that?

@kingthorin
Copy link
Collaborator

@bodiam do you recall a particular part of the file that wasn't working prior to this? I guess I could roll this back on my local copy and see how it fails but if it's intermittent then that's kind of a pain.

@bodiam
Copy link
Contributor Author

bodiam commented Apr 13, 2023

@kingthorin just remove some items and create a list with empty elements in YAML, that should do it

@kingthorin
Copy link
Collaborator

So with the abstracted tests we do already test that the value is not empty and is within the set. But there's not currently a way to test the full set.

Is there a simple way to return all (or n) unique values for a given faker? (Ex: All aircraft, or all first names .... I understand for ID numbers etc a full list might not be good hence "or n"). Then I could add tests that ensure that doesn't contain any empty entries?

Or, should we tackle this a different way and go lower and simply disallow/skip "reading" empty strings from the yaml?

@kingthorin
Copy link
Collaborator

Do we not have spotless:check running as part of our CI? Because I can't get it to pass locally. It seems upset about some of the Java 17 upgrades that have been done.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.833 s
[INFO] Finished at: 2023-04-15T18:43:10-04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.35.0:check (default-cli) on project datafaker: Unable to format file C:\pathtoworkspace\datafaker\src\main\java\net\datafaker\idnumbers\NricNumber.java: java.lang.reflect.InvocationTargetException: 19:10: error: illegal start of expression -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

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.

3 participants