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

[ JSTEP-10 ] Migrate smile module tests to JUnit 5 #553

Conversation

JooHyukKim
Copy link
Member

related #547

Blocked by #548

@JooHyukKim JooHyukKim marked this pull request as draft January 12, 2025 04:38
Copy link
Member Author

@JooHyukKim JooHyukKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok

@JooHyukKim JooHyukKim marked this pull request as ready for review January 12, 2025 06:20
Comment on lines +3 to +4
import java.io.ByteArrayOutputStream;
import java.io.IOException;
Copy link
Member Author

@JooHyukKim JooHyukKim Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cowtowncoder One thing tho, IntelliJ doesn't provide per-package wildcard handling 🤔 minimum 3 required to use wildcard. But with that trade off, we get to order imports Jackson way that I configured like below (There are more above and below)

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ideally we'd usually left imports as-is and avoid mechanical changes.

But FWTW, the usual cut-off is 5-or-more, I think. Then again for some packages (java.io, java.util) could use star-include for lower numbers.

I think that order of imports is more important than exact threshold for start includes, if that helps? (i.e. I'd rather have strict(er) ordering than worry about explicit vs star inclusion, if only one can be restricted).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to hear that ordering comes first!
Though I hope merging into master doesn't complicate as much tho

@cowtowncoder cowtowncoder merged commit ec58aab into FasterXML:2.19 Jan 14, 2025
0 of 4 checks passed
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