| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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. |
Sorry, something went wrong.
|
Every change should be compatible with Java 8. This can be verified by either compling with Java 8 or using the --release 8 javac flag in Java 9 or higher. Yeah, I might have gone a bit too far 😅 I am open to drop some unnecessary commits |
Sorry, something went wrong.
|
Looks like test_concat and test_random are failing unexpectedly. Ideas? |
Sorry, something went wrong.
|
I guess the test_random is failing because of the switch vom java.util.Random to java.security.SecureRandom? |
Sorry, something went wrong.
|
The concat test seems to fail due to a buggy refactoring |
Sorry, something went wrong.
Please make the switch to SecureRandom a separate PR:
|
Sorry, something went wrong.
|
I removed some of the more stylistic refactorings and the switch to SecureRandom. Now the diff of the changeset is about half as big |
Sorry, something went wrong.
|
Ah, the test test_fileutil_wrap_outputstream_default_textmode fails because setting the system property line.separator in jython/Lib/test/test_java_integration.py Line 169 in 3760bfb |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
These are mostly automated refactorings, best review each commit