Skip to content

Conversation

@TamasSzigeti
Copy link
Contributor

Fixes #1064

@what-the-diff
Copy link

what-the-diff bot commented Jan 24, 2024

PR Summary

This Pull Request involves a series of updates focused on cleaning up data in our codebase. It has systematically removed specific elements from various lists throughout the code. Essentially, this action is akin to deleting unused or unnecessary items from a spreadsheet to help streamline and improve efficiency.

Here's a breakdown of the key changes:

  • Cleanup Operation
    This update has removed several items from different lists across multiple lines of code. This process should help make our data cleaner, and potentially reduce clutter and improve the performance of our systems.

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (73ae126) 92.29% compared to head (affa80a) 92.42%.
Report is 5 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1066      +/-   ##
============================================
+ Coverage     92.29%   92.42%   +0.12%     
- Complexity     2817     2822       +5     
============================================
  Files           292      292              
  Lines          5607     5608       +1     
  Branches        599      599              
============================================
+ Hits           5175     5183       +8     
+ Misses          276      272       -4     
+ Partials        156      153       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kingthorin
Copy link
Collaborator

kingthorin commented Jan 24, 2024

@TamasSzigeti would you be up for adding tests to this or is that asking too much? (Not a trick, we can do it separately if that's outside your interest/skillset.)

@TamasSzigeti
Copy link
Contributor Author

Sure, gladly, will make time for it in the coming days.

@kingthorin
Copy link
Collaborator

Even just something like:

Get the List from the yaml: List<String> loremWords = getBaseList("lorem.words");
Create a Set from the List (since Sets in Java are unique values): Set<String> uniqueWords = new HashSet<>(loremWords);

Then assert that the two sizes should be equal, then we know there's no duplicates.
Ummm actually you could even do a parameterized test that passes "lorem.words" and "lorem.supplemental" as key String, use "Terms" instead of "Words" for the variable names, then it's just a single test method like wordListsShouldNotHaveDuplicates or something like that.

@kingthorin
Copy link
Collaborator

Your method content will be different but here's an example how to annotate and set things up:

@ParameterizedTest
@ValueSource(strings = {"null", "", "month", "year", "week"})
void invalidDuration(String invalid) {
assertThatThrownBy(() -> faker.date().duration(faker.random().nextLong(), invalid))
.isInstanceOf(IllegalArgumentException.class);
}

@TamasSzigeti
Copy link
Contributor Author

@kingthorin I did a bit more than that, have a look pls

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.

LGTM

@kingthorin
Copy link
Collaborator

Thanks!

@TamasSzigeti TamasSzigeti changed the title Remove duplicates from lorem yaml Remove duplicates from yaml values Jan 25, 2024
@kingthorin
Copy link
Collaborator

@snuyanzin @bodiam one of you wanna make a second call on this. I'm not 100% sure all lists should be unique. (I don't see a reason why not, but don't wanna kill something being unaware.)

@snuyanzin
Copy link
Collaborator

thanks for the contribution
i think there are a couple of thing need to be done

  1. apply spotless to remove unused imports
  2. there are several tests failing after this change, they should be adopted

@kingthorin
Copy link
Collaborator

Thanks!

@kingthorin kingthorin merged commit c8a53eb into datafaker-net:main Jan 26, 2024
@TamasSzigeti
Copy link
Contributor Author

Thank you too! Could you tag and publish a release, please?

@bodiam
Copy link
Contributor

bodiam commented Jan 27, 2024

@TamasSzigeti a snapshot release should have been published automatically, I can make a new official release soon if you need.

@kingthorin
Copy link
Collaborator

@TamasSzigeti
Copy link
Contributor Author

Thanks, I can use the snapshot for now, no problem

@TamasSzigeti TamasSzigeti deleted the remove-lipsum-duplicates branch January 27, 2024 10:22
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.

Lorem words are not unique

5 participants