Skip to content

Do not cache methods of expressions with args #719

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

Conversation

valfirst
Copy link
Contributor

@valfirst valfirst commented Feb 25, 2023

Fixes #716

@what-the-diff
Copy link

what-the-diff bot commented Feb 25, 2023

  • The method resolveExpression() was changed.
  • In the first part of this method, it checks if there is a dot in directive and args length equals to 0, then invoke current object's methods with no parameters; otherwise check whether there are any providers that can be invoked by using reflection mechanism (getProvider(), getMethod()) or not. If yes, return the result after invoking these methods; else call another function named "resolveFromMethodOn" which will try to find out corresponding functions from context objects' class files and cache them for future use(expression2function).
  • In the second part of this method: if dotIndex >=0 or current == null , just return directive directly as resolved value; else split string into two parts based on index number returned by getDotIndex(). Then search provider name in fakerContextMap according to left side substring before '.' character and right side substring after '.' character respectively . Finally invoke related provider's methods with arguments passed in through parameter list args[].

@codecov-commenter
Copy link

Codecov Report

Merging #719 (0d17b6c) into main (66feee4) will decrease coverage by 0.04%.
The diff coverage is 86.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #719      +/-   ##
============================================
- Coverage     92.89%   92.85%   -0.04%     
+ Complexity     2580     2579       -1     
============================================
  Files           281      281              
  Lines          5108     5109       +1     
  Branches        529      528       -1     
============================================
- Hits           4745     4744       -1     
- Misses          234      236       +2     
  Partials        129      129              
Impacted Files Coverage Δ
.../java/net/datafaker/service/FakeValuesService.java 84.70% <86.66%> (+0.42%) ⬆️
.../main/java/net/datafaker/providers/base/Lorem.java 93.75% <0.00%> (-6.25%) ⬇️
...ain/java/net/datafaker/idnumbers/KoKrIdNumber.java 81.25% <0.00%> (-6.25%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bodiam
Copy link
Contributor

bodiam commented Feb 25, 2023

Thanks! Do you think this is worth an 1.8.1 release?

@bodiam bodiam merged commit a0ad4df into datafaker-net:main Feb 25, 2023
@valfirst
Copy link
Contributor Author

@bodiam yes, it makes sense from my point of view

@valfirst valfirst deleted the do-not-cache-methods-of-expressions-with-args branch February 26, 2023 07:19
@valfirst
Copy link
Contributor Author

@bodiam is there any chance to release a patch version?

@bodiam
Copy link
Contributor

bodiam commented Mar 13, 2023

Yeah. I'll give it a shot this weekend!

@bodiam
Copy link
Contributor

bodiam commented Mar 15, 2023

1.8.1 with this fix should be available now.

@valfirst
Copy link
Contributor Author

@bodiam thank you!

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.

java.lang.IllegalArgumentException is thrown on expressions with optional args
3 participants