FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Some refactorings by thetric · Pull Request #393 · jython/jython · GitHub

/ jython Public

Some refactorings - #393

Open
thetric wants to merge 26 commits into
jython:masterfrom
thetric:some-refactorings
Open

Some refactorings#393
thetric wants to merge 26 commits into
jython:masterfrom
thetric:some-refactorings

Conversation

thetric commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

These are mostly automated refactorings, best review each commit

Stewori commented Aug 4, 2025

Copy link
Copy Markdown
Member

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.

thetric commented Aug 4, 2025

Copy link
Copy Markdown
Contributor Author

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

Stewori commented Aug 9, 2025

Copy link
Copy Markdown
Member

Looks like test_concat and test_random are failing unexpectedly. Ideas?

thetric commented Aug 9, 2025

Copy link
Copy Markdown
Contributor Author

I guess the test_random is failing because of the switch vom java.util.Random to java.security.SecureRandom?

thetric commented Aug 9, 2025

Copy link
Copy Markdown
Contributor Author

The concat test seems to fail due to a buggy refactoring

Stewori commented Aug 9, 2025

Copy link
Copy Markdown
Member

I guess the test_random is failing because of the switch vom java.util.Random to java.security.SecureRandom?

Please make the switch to SecureRandom a separate PR:

  • the test failure indicates it might be more than a refactoring
  • it relieves this PR (wich is already inconveniently big) by one test failure
  • the decision about that switch and handling the test failure would be disentangled from this PR

thetric force-pushed the some-refactorings branch 2 times, most recently from df1c236 to a4f4316 Compare August 10, 2025 08:14

thetric commented Aug 10, 2025

Copy link
Copy Markdown
Contributor Author

I removed some of the more stylistic refactorings and the switch to SecureRandom. Now the diff of the changeset is about half as big

thetric commented Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

Ah, the test test_fileutil_wrap_outputstream_default_textmode fails because setting the system property line.separator in

old_linesep = System.setProperty("line.separator", "\r\n")
now has no effect

Stewori commented Aug 12, 2025

Copy link
Copy Markdown
Member

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.

thetric force-pushed the some-refactorings branch from a4f4316 to 7353b2d Compare August 15, 2025 18:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

2 participants


Back | FazBrowse Home | New Git URL