Skip to content

Use RgxGen to cope with StackOverFlow#1092

Merged
snuyanzin merged 2 commits intodatafaker-net:mainfrom
snuyanzin:1091
Feb 26, 2024
Merged

Use RgxGen to cope with StackOverFlow#1092
snuyanzin merged 2 commits intodatafaker-net:mainfrom
snuyanzin:1091

Conversation

@snuyanzin
Copy link
Collaborator

fixes #1091

@what-the-diff
Copy link

what-the-diff bot commented Feb 22, 2024

PR Summary

  • Update of Dependency in pom.xml
    The software dependency generex was replaced by another software component rgxgen of version 2.0-SNAPSHOT.

  • Updated Module Dependency
    The net.datafaker module, which is one of the parts of our software, now requires rgxgen instead of generex.

  • Updated Implementation in FakeValuesService
    The FakeValuesService class is now using RgxGen class methods from the package com.github.curiousoddman.rgxgen

  • Modified regexify Method in FakeValuesService
    The method regexify in the FakeValuesService class is now implementing RgxGen instead of Generex. This change will improve the way our software is using regex (short for Regular expressions - they help us find specific information in large amounts of data).

  • New Test Case Added
    A test case called issue1091 was added to validate the behavior of the updated regexify method in the FakeValuesService class. It will help us ensure that the updated method behaves as expected with specific regex patterns.

pom.xml Outdated
<commons-text.version>1.11.0</commons-text.version>
<commons-validator.version>1.8.0</commons-validator.version>
<generex.version>1.0.2</generex.version>
<rgxgen.version>2.0-SNAPSHOT</rgxgen.version>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like they also do not release often however provide snapshots...
Anyway need this version since the leveraging api is not supported in previous non-SNAPSHOT

@bodiam
Copy link
Contributor

bodiam commented Feb 22, 2024

I don't think we can depend on snapshot versions. If they publish a new version, we would have no idea what kind of code we get into our library. I don't think this would be an acceptable situation.

@snuyanzin
Copy link
Collaborator Author

based on the commit history they are not going to do it...
the latest commit was more than 8 months ago...

@bodiam
Copy link
Contributor

bodiam commented Feb 22, 2024

I don't really care how often they release, I care that we don't depend on snapshots for production releases. Maybe they change the api in a year, then it would break every Datafaker release depending on the snapshot, including older versions. If they would introduce something malicious, we would download this too automatically. So no, let's not do this.

@kingthorin
Copy link
Collaborator

Wouldn't that be true of any dependency?

@bodiam
Copy link
Contributor

bodiam commented Feb 22, 2024

No. Once a dependency with a version has been published on Maven Central, it's not possible to change the library anymore. The same is not true for snapshots.

Also: curious-odd-man/RgxGen#93

@bodiam
Copy link
Contributor

bodiam commented Feb 22, 2024

Also, we wouldn't be able to publish Datafaker anymore, cause the maven release plugin forbids releases with snapshot dependencies (unless you override it, but that's just not a great idea)

@kingthorin
Copy link
Collaborator

Okay, fair. Let's see what they say. Nothing wrong with asking.

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.

Assuming a release happens and this PR goes forward then I think src/main/java/module-info.java will also need updating.

@snuyanzin
Copy link
Collaborator Author

it is already a a part of the PR or do you mean something else?

@kingthorin
Copy link
Collaborator

All good, I totally overlooked it 🤦‍♂️

@bodiam
Copy link
Contributor

bodiam commented Feb 25, 2024

2.0.0 has been released!

@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (b37c566) to head (9ee5015).
Report is 12 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    #1092      +/-   ##
============================================
- Coverage     92.35%   92.33%   -0.02%     
  Complexity     2821     2821              
============================================
  Files           292      292              
  Lines          5609     5610       +1     
  Branches        599      598       -1     
============================================
  Hits           5180     5180              
  Misses          275      275              
- Partials        154      155       +1     

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

@snuyanzin
Copy link
Collaborator Author

thanks for letting know, updated to use released version

@snuyanzin
Copy link
Collaborator Author

Merging since now there is no SNAPSHOT version in deps

@snuyanzin snuyanzin merged commit c75b24e into datafaker-net:main Feb 26, 2024
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.

StackOverflowError for regexify

4 participants