esanchezros / quickfixj-spring-boot-starter

Spring Boot Starter for QuickFIX/J
Apache License 2.0
125 stars 57 forks source link

Quickfixj messages dependencies added by default #131

Closed Svanar closed 3 months ago

Svanar commented 4 months ago

Hi, I started using your starter and I'm having a blast. However I have one question and would love your feedback on it. What do you think about removing quickfixj-messages-all dependency and adding section in README that it should be added like quickfix-j is doing https://github.com/quickfix-j/quickfixj?tab=readme-ov-file#quickfixj-runtime. The reason I'm thinking of removing it is that it adds a lot of duplicated classes by default and, for example, in my case we are using only 4.4 version, so I'm not interested in any other classes.

While this is not really an issue, I think that removing of those dependencies would make writing code a bit simpler, because one does not have to scroll through imports to check what exactly is used, IDE will index and search faster.

Looking forward to your feedback. Thanks!

esanchezros commented 4 months ago

Hi @Svanar, Thanks for using the starter and for your suggestion. The initial idea of the starter was to reduce the boilerplate configuration needed to start an initiator and an acceptor, but also include all the required dependencies required by quickfixj.

I understand your point and perhaps a different approach would be to add each library with the specific version to the starter's dependency management with scope optional and then allow the user to pick the specific dependency(ies) they want to use.

Svanar commented 4 months ago

Thank you for your response. I think that adding optional would resolve all inconveniences I mentioned. I know that it is possible to exclude some packages from my app pom, but making it not added in the first place would be really really nice in my opinion.

esanchezros commented 3 months ago

@Svanar would you mind having a look at this PR https://github.com/esanchezros/quickfixj-spring-boot-starter/pull/133?

Svanar commented 3 months ago

@esanchezros I checked and I think that it is exactly what I was thinking about. Thank you :) Just a though - maybe you should add something to readme that end user should include version that will be used?

esanchezros commented 3 months ago

Yes, working on that at the moment. It's a breaking change too so I need to bump the minor version too

esanchezros commented 3 months ago

@Svanar I've added a section to the readme explaining the change and will add the breaking change in the release notes. Will you be able to review again and approve the PR please? Thanks

esanchezros commented 3 months ago

Available from version 2.18.0

Svanar commented 3 months ago

@esanchezros Appologies, I was away from my computer for couple of days and just got back to it. I must say that I really like the change and appreciate your work. Huge thank you

esanchezros commented 3 months ago

No worries @Svanar, let me know if you find any issues with the new version