Skip to content

Refactor FakeValues#1335

Merged
asolntsev merged 4 commits into
mainfrom
refactoring/fake-values
Aug 23, 2024
Merged

Refactor FakeValues#1335
asolntsev merged 4 commits into
mainfrom
refactoring/fake-values

Conversation

@asolntsev
Copy link
Copy Markdown
Collaborator

No description provided.

Class FakeValues called FakeValuesContext.setPaths(), but these "paths" are only used by FakeValues itself.
Now FakeValuesContext holds only a single immutable "path", and FakeValues either uses it, or calculates "paths" if it's null.

P.S. Removed method fakeValues.supportsPath() because it was not used.
@asolntsev asolntsev self-assigned this Aug 23, 2024
@what-the-diff
Copy link
Copy Markdown

what-the-diff Bot commented Aug 23, 2024

PR Summary

  • Introduced Lazy Evaluation
    A new helper class LazyEvaluated has been introduced for handling data that doesn't need to be calculated until it's actually used.
  • Refactored Company class
    The Company class has been improved. We've removed redundant fields, and introduced efficient methods and ways to select buzzwords.
  • Updated Values Handling in FakeValues
    The way we handle values in FakeValues class has significantly been improved. We've introduced lazy evaluation, removed redundancy and added new private method to efficiently load values from different sources.
  • Simplified FakeValuesContext
    The FakeValuesContext class has been simplified - specifically we just renamed a field and made corresponding updates.
  • Improved FakeValuesGrouping
    A method in FakeValuesGrouping now uses an updated getter method from FakeValues.
  • Enhanced FakeValuesService
    Various improvements have been made in FakeValuesService, including more efficient string concatenation, better handling of nested objects and method refactors for better type safety.
  • Updated Tests
    Numerous test classes have been updated to be compatible with these changes. Certain redundant tests have been removed, and assertions have been improved for better type compatibility.

Instead of `Object`, method fetchObject() now returns `T`.
So clients of fetchObject() don't need to cast it to needed type.
@asolntsev asolntsev force-pushed the refactoring/fake-values branch from c3a5ad2 to ca4091d Compare August 23, 2024 16:30
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.15%. Comparing base (1f346c7) to head (46102cd).
Report is 4 commits behind head on main.

Files Patch % Lines
...rc/main/java/net/datafaker/service/FakeValues.java 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1335      +/-   ##
============================================
+ Coverage     92.04%   92.15%   +0.11%     
- Complexity     3155     3156       +1     
============================================
  Files           316      317       +1     
  Lines          6232     6209      -23     
  Branches        634      623      -11     
============================================
- Hits           5736     5722      -14     
+ Misses          337      334       -3     
+ Partials        159      153       -6     

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


private List<String> loadBuzzwords() {
List<List<String>> buzzwordLists = faker.fakeValuesService().fetchObject("company.buzzwords", faker.getContext());
return buzzwordLists.stream().flatMap(Collection::stream).toList();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why stream it then map it to a stream then collect it? Why not just:
buzzwordLists.stream().collect(Collectors.toList())

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no, buzzwordLists.stream().collect(Collectors.toList()) is the same as just buzzwordLists.

The point is that buzzwordLists is list of lists. And we need to convert it to a list.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay, devil in the detail 👿

this allows downloading test report from GitHub actions in case of test failure.
@asolntsev asolntsev merged commit b2ebdc1 into main Aug 23, 2024
@asolntsev asolntsev deleted the refactoring/fake-values branch August 23, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants