Skip to content
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

WIP Rewrite selection code #870

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

hazendaz
Copy link
Collaborator

Code is invalid, confirmed on mybatis project that licensing incredibly speeds up by fixing this and still does what it should. Tests currently fail as they are hard-coded to LF not OS dependent. Need to fix that but pushing up to get a review.

Code is invalid, confirmed on mybatis project that licensing incredibly speeds up by fixing this and still does what it should.  Tests currently fail as they are hard-coded to LF not OS dependent.  Need to fix that but pushing up to get a review.
@hazendaz
Copy link
Collaborator Author

See #859 that found the issue. Tests are currently hard-code to LF so they fail on windows. I presume will work here on *nix but haven't confirmed at time of writing. I used the build by skipping tests on mybatis which was taking minutes to process, now seems faster but need to do more testing to confirm.

@mathieucarbou Can you look this over to see if I'm on the right path here?

@hazendaz hazendaz marked this pull request as draft December 26, 2024 02:16
@hazendaz
Copy link
Collaborator Author

ok ignore my mybatis test results. Variance is a windows thing so it doesn't actually improve with this for that project. Seems rather something on my machine changed so I was getting better results but retesting its master I get seconds of same result so this has no improvement there. Will it improve projects with node_modules, probably but its a read so I doubt it changes much. I still think original logic is indeed incorrect but not sure what was in mind back in 2020 and regardless issue I thought this solved I think is longer standing than 2020.

@hazendaz
Copy link
Collaborator Author

ok GHA confirmed. Tests need simply updated to work cross platform. Provided this is on right track, will address the tests afterwards. *nix worked fine per GHA.

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.

1 participant