search

studerw / td-ameritrade-client

TD Ameritrade Java Client
Apache License 2.0
69 stars 47 forks source link

Introduce Lombok? #12

Open vphilipnyc opened 4 years ago

vphilipnyc commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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