Skip to content

Feat: allow args filtering#313

Merged
AloisSeckar merged 8 commits intoAloisSeckar:masterfrom
ab-schneider:feat/allow-args-filtering
Oct 27, 2025
Merged

Feat: allow args filtering#313
AloisSeckar merged 8 commits intoAloisSeckar:masterfrom
ab-schneider:feat/allow-args-filtering

Conversation

@ab-schneider
Copy link
Contributor

@ab-schneider ab-schneider commented Oct 26, 2025

🧩 Summary

  • Refactored demo list initialization: replaced demo set with Map<Integer, IDemo> (JEP_DEMO), allowing future filtering by arguments.

  • Added ArgFilterUtil class containing all argument parsing and filtering logic.

  • In Main, demos are now filtered and sorted by JDK and JEP number before execution.

🔧 Remaining tasks

  • Update older JEPs (from java11 and java12 folders) to fit the new structure — currently commented out.

  • Update documentation and contribution guide to align with the new structure and filtering logic.

  • Add unit tests — filters were tested manually and work correctly, but bunch of unit tests would be valuable to check different filter scenarios and have a base for future extension.

💬 Note

If you agree with the new structure, I suggest merging this PR and handle the remaining points in a follow-up issue (or several smaller ones). Otherwise continue in this PR will lead to more merge conflicts.

Closes #268 #270 #271

@AloisSeckar
Copy link
Owner

Hi @ab-schneider. First of all, really big thanks for putting this together. I already like it.

I am just not sure about one thing - if it is wise to load all demos inside JEPInfo.java class. I see your approach works, but I would be concerned about growing list of JEPs. We are saving amount of separate files, but the one file will be longer and longer. And constant souce of merge issues until all JEPs are covered.

I would rather keep a file for each JDK version. But I am not 100% convinced. What is your reasoning for putting everything in one file?

@ab-schneider
Copy link
Contributor Author

Thanks for the feedback @AloisSeckar

My intention was to keep things a bit cleaner — I didn’t really like the idea of having multiple almost-empty classes that only fill the map and don’t do anything else.

Following the single responsibility principle, I thought it made sense to have one class responsible for initializing both demos and their metadata.

But I didn’t fully consider the merge-conflict angle, and you’re right — a single growing file could easily become a hotspot.

I’ll quickly change it back to multiple classes.

Copy link
Owner

@AloisSeckar AloisSeckar left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. I know it also have cons, like having more files to look for, but on the other hand they are rather short and once completed, they shouldn't change much, unless we redesign whole structure again.

I just think we're now missing Java13DemoLoader.java. Although no JEPs so far, it can be already prepared.

Plus I put 3 other remarks in comments to think about, please, have a look.

@ab-schneider ab-schneider force-pushed the feat/allow-args-filtering branch from b1c17b0 to c6d12b8 Compare October 27, 2025 08:55
@AloisSeckar
Copy link
Owner

Lets merge and see how it turns out :)

Big thanks for this contribution ❤️

@AloisSeckar AloisSeckar merged commit 14542ff into AloisSeckar:master Oct 27, 2025
1 check 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.

Allow option to skip all "linking" demos

3 participants