Skip to content

Conversation

thetric
Copy link
Contributor

@thetric thetric commented Aug 4, 2025

These are mostly automated refactorings, best review each commit

@Stewori
Copy link
Member

Stewori commented Aug 4, 2025

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
Copy link
Contributor Author

thetric commented Aug 4, 2025

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
Copy link
Member

Stewori commented Aug 9, 2025

Looks like test_concat and test_random are failing unexpectedly. Ideas?

@thetric
Copy link
Contributor Author

thetric commented Aug 9, 2025

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

@thetric
Copy link
Contributor Author

thetric commented Aug 9, 2025

The concat test seems to fail due to a buggy refactoring

@Stewori
Copy link
Member

Stewori commented Aug 9, 2025

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 thetric force-pushed the some-refactorings branch 2 times, most recently from df1c236 to a4f4316 Compare August 10, 2025 08:14
@thetric
Copy link
Contributor Author

thetric commented Aug 10, 2025

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
Copy link
Contributor Author

thetric commented Aug 11, 2025

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
Copy link
Member

Stewori commented Aug 12, 2025

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 thetric force-pushed the some-refactorings branch from a4f4316 to 7353b2d Compare August 15, 2025 18:00
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