-
Notifications
You must be signed in to change notification settings - Fork 224
Some refactorings #393
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
base: master
Are you sure you want to change the base?
Some refactorings #393
Conversation
I have mixed feelings about this. It fixes things that are not broken. It is a very big change set and we want to keep changes in Jython 2 minimal since it's kind of in maintenance mode. OTOH this clearly modernizes the code and most changes are justified. Some might even benefit maintainability. So, this might be mainly a matter of taste. Perhaps an unnecessary risk, but then we have tests to secure such changes. Then again, I wouldn't want someone to spend too much time (or time at all) on reviewing this PR since it doesn't really fix anything. A more substantial concern is that it probably bumps the minimally required Java version heavily. E.g. I don't think try-with-resources or replacing if with switch in that way would be supported on Java 8 (or 9). There are surely good reasons to increase the required Java version, but cosmetic changes are not among these. So, I'd say if it's only about the subset that leaves the currently required Java version unchanged, we might consider it. |
Every change should be compatible with Java 8. This can be verified by either compling with Java 8 or using the Yeah, I might have gone a bit too far 😅 I am open to drop some unnecessary commits |
Looks like |
I guess the |
The concat test seems to fail due to a buggy refactoring |
Please make the switch to
|
df1c236
to
a4f4316
Compare
I removed some of the more stylistic refactorings and the switch to SecureRandom. Now the diff of the changeset is about half as big |
Ah, the test jython/Lib/test/test_java_integration.py Line 169 in 3760bfb
|
Please move refactorings that cause test failures into separate PRs. I acknowledge that in some cases the tests might require adjustment, but that requires a more careful case by case analysis, better suited in individual PRs. |
a4f4316
to
7353b2d
Compare
These are mostly automated refactorings, best review each commit