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

Introduce Lombok? #12

Open
vphilipnyc opened this issue May 21, 2020 · 4 comments
Open

Introduce Lombok? #12

vphilipnyc opened this issue May 21, 2020 · 4 comments

Comments

@vphilipnyc
Copy link
Contributor

Hi William,

Would you be open to introducing Lombok as a dependency to the project? It would remove quite a bit of boilerplate code with your beans (classes under your "domain" folder).

I don't mind contributing a PR for this change - it would make things easier to read and less error-prone. I've used it on several projects (in prod too).

https://projectlombok.org

Thanks,
Victor

@studerw
Copy link
Owner

studerw commented May 23, 2020

I used this tool to generate most of the model classes: jsonschema2pojo. It makes the pojos okay, but the errors I've had were from TDA's schema isn't 100% accurate. So far I've caught most of them except for some of the responses to really complex trades that I've never tried, and actually don't even understand, lol.

Right now I have a couple types (e.g. order.cancelTime) that need special Jackson annotations, so if that could still be configured, I don't mind. You could do a PR for just switching to Lombock on, e.g., com.studerw.tda.model.account.Order, and I'll take a look.

@vphilipnyc
Copy link
Contributor Author

vphilipnyc commented May 26, 2020

Sounds fair. Let me come back to this. Looking at the codebase closer, I want to make sure Lombok and the JSON annotations don't cause any issues together.

The other consideration is that if you're planning on using the jsonschema2pojo tool each time there's a change with the TD API, then you'd be essentially undoing the Lombok change (and optionally re-applying them to reflect the TD API). Not trying to cause any headaches, but completely fine if Lombok is not a direction you'd want to take.

It would likely work really well for the PriceHistReq class, for example.

@vphilipnyc
Copy link
Contributor Author

Also, I don't believe introducing Lombok would affect serialization, but if changes are made that are incompatible with serialization (e.g., fields are removed, types are changed, etc), then it could turn into a hassle. Just not sure whether this is a valid concern depending on how the project is used currently.

@studerw
Copy link
Owner

studerw commented Jul 1, 2020

Thanks, @vphilipnyc! I'll try to introduce this to the next release

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

No branches or pull requests

2 participants