knowm / XChange

XChange is a Java library providing a streamlined API for interacting with 60+ Bitcoin and Altcoin exchanges providing a consistent interface for trading and accessing market data.
http://knowm.org/open-source/xchange/
MIT License
3.81k stars 1.93k forks source link

Let's reconsider lombok #2342

Open brilliantnut opened 6 years ago

brilliantnut commented 6 years ago

I saw the old discussion on the #2008 and #2066 around removing the lombok dependency, and do understand your reluctance to include a number of 3rd party libraries, but I want to make the case for lombok again.

In my recent java development, lombok has become so common, that I almost consider it a part of the language. It ends up reducing a huge amount of boiler-plate code, and also reduces maintenance overhead - e.g. for toString(), equals, hashcode and constructors.

In most of my other projects, my DTOs look like this:

import lombok.Data

@Data
public class DepositAddress {
  private final String address;
  private final String destinationTag;
}

and that's it. @Data gives you an RequiredArgs Constructor, Getters, Setters (for non-final fields), equals, hashcode, toString. If you end up adding, removing or moving around fields, the generated toString / equals / hashcode changes automatically.

Also, most IDEs (definitely Eclipse and Idea) now have first class support for lombok, so generated getters / setters & constructors show up correctly while coding in other classes.

https://github.com/timmolter/XChange/pull/2008/commits/7f7b162cbfa434cc3d4196a93e0d0b98bd13f831 is actually a good representation of the before / after state of using lombok.

bryantharris commented 6 years ago

I'm a big fan of Lombok as well. It also shouldn't be a run time requirement, just a build time requirement. Also some one time fiddling with your IDE to recognize.

As with most approaches that remove boiler plate, it's tough to go back once you accept it as part of your development toolset.

jnorthrup commented 6 years ago

I think boilerplate removal is important as well. i have utilized kotlin with swagger-codegen to minimize the maintenance footprint in a particular PR, but this is the same basic thing.

rafalkrupinski commented 6 years ago

I've only seen 1 abandoned project using lombok. Requires setting up IDE and java cmdline tools fo every developer and CI environment.

jnorthrup commented 6 years ago

java as a language has peaked. you can be grateful when a project is not overrun with Spring and dependency injection as those tend to derail your ability to look at the code and understand what's happening at runtime because of the isolated implementation code bound from reflection and assemblies. my personal favorite lombok-like thing was google autogen which does the exact same thing.

https://www.youtube.com/watch?v=Q_Z_8HXt68k is an excellent take on the evolution of jvm options and what's emerging pragmatically

brilliantnut commented 6 years ago

@rafalkrupinski wrote:

I've only seen 1 abandoned project using lombok. Requires setting up IDE and java cmdline tools fo every developer and CI environment.

This does describe older versions of lombok, but newer versions abstract away a bunch of this friction. However, it almost sounds like you're saying the abandoned project that you saw was because of lombok? That sounds interesting, and I'd love to hear how lombok contributed to the abandonment of that project as an interesting other-side argument against using lombok. I'm interested to disconfirm my bias.

@jnorthrup I think I hear both sides of the argument in your two posts, but I'll take your vote as a positive one for now. 😄

@timmolter How many votes before we actually move forward with this, one way or another? I'm happy to pick up the initial set of changes, and then it becomes a part of the toolset right?


Disclaimer: I'm not related to the lombok project any more than as a user.

npomfret commented 6 years ago

Sorry. I'd vote against it for now.

jnorthrup commented 6 years ago

@timmolter has been standing his ground, defending exactly what is in the codebase, with no lattitude for any discussion about approach.

the problems I have are that the jaxrs tier is exclusive to XChange, but is an external dependency, presumably one of tim's drinking/hackathon buddies, and lately that has started to interfere with the success of REST authentication that works against java.net.URL connection. XChange is the sole consumer of si.mmazi.rescu in google

and then hey, if you can generate 5 implementations in two weeks by any other methods, in tim's policies, that belongs in some other project, evidence that's basically where I'm at using swagger as the backbone of API specification to work with the XChange-core baseclasses.

I have given Tim's approach the beneift of the doubt, and tried to look at the factors, and if you want the current hand-maintained implementations, each in thier own tedious but unneccesary maven reactor project, this works.

i agree that maven as a build tool is an excellent scaffolding for code generation and release, but this project doesn't show any examples of using maven's power to a benefit, it's just a build hierarchy and it takes ...forever... to build all the seperate modules which each consist of 5-10 classes and some dto's

what i did find is that if you collapse the codebase into one maven project, it compiles pretty quick, and THEN, when you get into things like code generation etc. you find a valid reason to start teasing out maven reactor projects.

I offered to Tim to enlist me as a maintainer of the project so that evolutions and submissions can receive the benefit of someone who still spends energy in implementing modules, and i've noticed in response he's closed out my PR with code generation.

this is not the only project with intractable core developers, if you've ever talked to any bitcoin development, you will get swiftly attacked for proposals of any kind. even the most likely developer to be satoshi nakamoto, gavin, has been tossed out of core bitcoin dev.

i wouldn't expect an answer from Tim, he appears to proceed without debate on all aspects of this project.

rafalkrupinski commented 6 years ago

If it makes you feel better, I wouldn't start a new project with Java, unless for compatibility. And grandpa Maven? LOL!

timmolter commented 6 years ago

I don't see a real big benefit of introducing lombok at this point. Is just reducing boilerplate code really the only benefit? What about potential drawbacks such as:

  1. incompatibility with jackson marshalling and unmarshalling
  2. Build time
  3. Inconsistency across all modules - some use lombok others not. Do we now need to enforce that people use lombok for all PRs? Or is it an optional?
  4. Newcomers without lombok experience (even if it is easy) will need to learn one additional thing
  5. Who will go and remove all the boilerplate code in the entire project?
  6. Android compatibility issues
  7. Java 9/10 issues
  8. IDE issues
  9. Edge cases in the code where things break (and there is a looooooooot of code in XChange right now)

Since I never used lombok, I don't have a sense if any of the above issues will arise or not. It seems that the potential risks outweigh the rewards.

jnorthrup commented 6 years ago

i think it's worthy of a POC, where he who smelt it dealt it. deleting accessors in regex is not really hard work. can be done with reflection code all at once to regenerate pojos as well. 99% of the jackson annotations are unnecessary defaults

brilliantnut commented 6 years ago

@timmolter :

  1. incompatibility with jackson marshalling and unmarshalling

I'm not sure what incompatibility you're talking about. In my personal code, I regularly use Jackson with lombok'd data classes.

  1. Build time

The difference is build time is not even going to be measurable, but we could do a comparative build between the develop branch and the lombok poc branch to confirm that the build time is not affected.

  1. Inconsistency across all modules - some use lombok others not. Do we now need to enforce that people use lombok for all PRs? Or is it an optional?

It would be optional, even within the same module - it is totally okay to have some classes written without any getters / setters (i.e. with lombok), and others written with explicit getters & setters. In fact, even if a class which has lombok annotations has an explicit constructor or method, the explicit method takes precedence.

If we wanted to move away from lombok in the future, it would be a 1-step process to remove lombok annotations, and generate delomboked code and commit again. See https://projectlombok.org/features/delombok

  1. Newcomers without lombok experience (even if it is easy) will need to learn one additional thing

Sure, but its easier, and much more widespread, than the http rest api connection library that we use (rescuu?)

  1. Who will go and remove all the boilerplate code in the entire project?

We don't have to go and remove all the boilerplate code right away. Just enabling lombok use, removing it from a few example classes, and gradually moving away would be a good idea.

  1. Android compatibility issues

What issues? https://projectlombok.org/setup/android

  1. Java 9/10 issues

Lombok has jdk9 support since last 3 releases, with support improving with each release. jdk10 support is already starting in their edge-release. Xchange is still tracking against jdk9 support, so I'm pretty sure we can work through the present support.

  1. IDE issues

One time IDE settings / plugins installation required. However, lombok has more IDE support (Eclipse, Idea, Netbeans) than Xchange has IDE format files (Eclipse, Idea).

  1. Edge cases in the code where things break (and there is a looooooooot of code in XChange right now)

Sure, so extensive testing is required, but the point of lombok is to reduce the amount of code in the project. In most cases, only the getters / setters / constructors are replaced, so not necessarily so hard / required to test.

@jnorthrup:

i think it's worthy of a POC, where he who smelt it dealt it. deleting accessors in regex is not really hard work. can be done with reflection code all at once to regenerate pojos as well. 99% of the jackson annotations are unnecessary defaults

Happy to pick up a POC, but will likely be weeks before I submit it.

npomfret commented 6 years ago

If getting rid of getters and setters is a goal, I'd prefer to just use constructors and public final fields to achieve this. Some objects that are perhaps complicated to construct could have additional builder classes or some helpful factory methods.

brilliantnut commented 6 years ago

@npomfret Going to public final fields is a one-way door, which causes the rest of your code to change when you change that decision. Suddenly if you want to have a getter, because the getter will do something different than just returning the field value (e.g. lazy load the value, or generate a default for nulls), you need to change the class, but you also need to change all client code, so I'm not fully supportive of moving to public final fields instead.

Lombok removes not just getters and setters, but can also be used to autogenerate Constructors and Builders. Even in the direction of final fields and immutable classes that you're talking about, lombok is helpful.

IanWorthington commented 6 years ago

I'm another content lombok user, happy to be able to see the real code better once all the boilerplate is stripped out. I see no reason why lomboked and non-lomboked code can't coexist. Anyone lomboking existing code needs to ensure that appropriate test cases exist or write them, or if they're reluctant to so do, leave alone.

There are some aspects of lombok that are controversial. But I don't see it difficult to define a subset that is not. Off the top of my head: @Getter, @ToString, @EqualsAndHashcode, @Slf4j, @NonNull strip out most of the boilerplate without harming legibility. I'm sure others can add to the list.

So, I would propose that xchange simply /tolerates/ specific lombok annotations but does not require them.

jnorthrup commented 6 years ago

I am maintaining branch swagger-gen and running bitmex, idex, huobipro, bitcoincoid, and stocks.exchange separately and happily. I welcome merciless refactorings because by definition, refactorings are intended to reduce complexity (tending towards increasing entropy, or reduced kolmogorov complexity, but that's a good thing)

in terms of technical debt, maintaining 1 line of code is worth some hypothetical amount of money on the java scale of expense for this project (vs cheaper js).

so here's the measure per module, no bias, whatever is checked in is considered maintained code. i tend to toss in a doc-file when it helps anchor things.

as a point of reference, idex implements all of the documented API's that XChange has methods for. different modules seems heavier than others.

imho (and in a seperate POC of my own), these modules are roughly ideal size for a package, but too small and uninteresting for a module, until you get into code generation matters where maven is doing the generation, and you want to prevent mistakes by doing too much maven in one place. so i lump them all together and they build instantly, except for my swagger modules which benefit from having the POM exclusively.

jim@XChange$ for i in xchange-* ; do echo $i: $( wc -l < <(cat `find $i -type f`)); done

xchange-abucoins: 5749 xchange-acx: 2431 xchange-anx: 7124 xchange-bibox: 3686 xchange-binance: 3307 xchange-bitbay: 1773 xchange-bitcoinaverage: 2387 xchange-bitcoincharts: 3882 xchange-bitcoincoid: 1705 xchange-bitcoincore: 457 xchange-bitcoinde: 12828 xchange-bitcoinium: 2324 xchange-bitcointoyou: 14068 xchange-bitcurex: 1960 xchange-bitfinex: 6954 xchange-bitflyer: 2770 xchange-bitmarket: 4387 xchange-bitmex: 5813 xchange-bitso: 1682 xchange-bitstamp: 28216 xchange-bittrex: 5051 xchange-bitz: 2650 xchange-bleutrade: 7100 xchange-blockchain: 3773 xchange-btcc: 455 xchange-btcmarkets: 3975 xchange-btctrade: 6237 xchange-btcturk: 18992 xchange-campbx: 1928 xchange-ccex: 24438 xchange-cexio: 3585 xchange-coinbase: 11927 xchange-coinegg: 2016 xchange-coinfloor: 3699 xchange-coinmarketcap: 19250 xchange-coinmate: 4762 xchange-core: 9667 xchange-cryptofacilities: 3205 xchange-cryptonit: 2321 xchange-cryptopia: 43374 xchange-dsx: 6625 xchange-empoex: 1715 xchange-examples: 14243 xchange-gatecoin: 3650 xchange-gateio: 3773 xchange-gdax: 3655 xchange-gemini: 4486 xchange-hitbtc: 11746 xchange-huobipro: 2913 xchange-idex: 4949 xchange-independentreserve: 2192 xchange-itbit: 2384 xchange-koineks: 791 xchange-koinim: 530 xchange-kraken: 20858 xchange-kucoin: 9742 xchange-kuna: 2189 xchange-lakebtc: 2328 xchange-liqui: 3030 xchange-livecoin: 5596 xchange-luno: 2568 xchange-mercadobitcoin: 10724 xchange-okcoin: 3773 xchange-openexchangerates: 2863 xchange-paribu: 513 xchange-paymium: 879 xchange-poloniex: 4673 xchange-quadrigacx: 1828 xchange-quoine: 3922 xchange-ripple: 6261 xchange-stocksexchange: 2594 xchange-taurus: 1766 xchange-therock: 2567 xchange-truefx: 495 xchange-vaultoro: 2117 xchange-vircurex: 1223 xchange-wex: 8086 xchange-yobit: 15098 xchange-zaif: 1734

IanWorthington commented 6 years ago

@jnorthrup -- I'm struggling slightly to interpret what you've written here. Are you suggesting that the cost of maintenance would be reduced by allowing lombok?

cymp commented 6 years ago

I've been using lombok for about a month now since you started the discussion here. Well it does remove some boiler plate code, but I can't see any must have features, only "fun stuff which can look annoying to anyone who knows nothing about lombok".

I would vote against but will not cry if it is accepted here.

IanWorthington commented 6 years ago

@cymp --

I didn't use it for a long time as it conflicted with my aspectj-based trace and timing package. (Or so I thought. I now gather there may be a better way of weaving the code.).

But in the end the nuisance of having to constantly generate and maintain getters and toStrings -- and to check which of the bucketload of getters and setters Java requires of me actually contain real code hidden in the swamp of boilerplate.

Having moved over to lombok I've now been able to delete all the swamp and what I see when I look at my code is just the non-default stuff.

Recommended.

jnorthrup commented 6 years ago

@IanWorthington yes the cost of maintenance would go down with apllied generation tools like lombok and you seem to inadvertently agree by citing the nuisance yourself (nuisances cost more not less)

jnorthrup commented 6 years ago

wrt aspects and di, the projects I've seen that leaned into these most heavily tend to only be simpler for the author and the library authors.. annotations processing let alone synthetic classes can derail the source of errors...

IanWorthington commented 6 years ago

@jnorthrup I'd certainly agree that there is a line to be drawn between stuff that makes maintenance easier and stuff that makes it more difficult, especially for an extended team of variable skills. I tend towards using the simplest solution for any job. I don't use all of lombok, and certainly not all of aspectj nor Spring.

npomfret commented 6 years ago

I really think just getting rid of all the getters and having public final fields is a much simpler solution.

Lombok feels like a solution to a problem we don't have if we embark on a little code clean up.

IanWorthington commented 6 years ago

@npomfret Sorry Nick, but that would be a really terrible idea, for two reasons:

  1. It would require changing all the code base to access the public fields rather than via the getters, for no benefit.

  2. The functionality of getters allows their internals to be changed at any stage without having to modify the code which calls them.

Allowing selected lombok features is a solution to the problem which involves no code clean up.

npomfret commented 6 years ago

1) is really not that hard, intellij will probably do it all for you

2) is something we don't do - we don't change how getters work in this project. they get, that's it. They return a field. We don't change them. In fact, I believe it's not strictly a getter if it does something other than return the value of the field that it's named after. Correct me if I'm wrong but that's a part of the bean spec from back in the day, no?

IanWorthington commented 6 years ago

@npomfret --

  1. What about non-project code. All that would have to be changed as well, breaking compatibility.

  2. I'm not sure if it would be strictly a getter then or not. Quite possibly not, given the enormous amount of crassness Gosling et. al. forced upon their language (don't get me started...). Notwithstanding, the encapsulation of functionality is a good thing, giving an easy way for code to evolve without breaking dependant code.

Personally I would often prototype a class using public fields, but I always end up changing the ones that work to have getters. lombok allows me to have that in from the outset with the barest minimum of effort.

I really don't see that this viable idea, especially when lombok getters can achieve the same without breaking code nor encapsulation, sorry.

jnorthrup commented 6 years ago

All great arguments to use kotlin or Lombok for purely metric simplification as they both seem to enhance the weight of intended lines of code and reduce the camouflage in the dto layers. I'm already content to do so and have.

jnorthrup commented 6 years ago

guys @timmolter has put out an RFC in #2452 and in that space of time 11 more PR's have piled up -- here's the chance.

There is some background on on this issue, #2432 and #2443 (first rolled into the second) have met approval -- and I have a more radical branch made more suitable for ... intellij automatic unit testing... and other things, at #2436

jamding commented 6 years ago

If generating case class-esque data objects is the primary value-add for adding a dependency on Lombok, its notoriously bad at handling subtypes in a generalizable way and is too opinionated in concurrent usages (vs Immutables or even Autovalue)

jnorthrup commented 6 years ago

i have recently extracted a great deal of XChange from the jaxrs metadata into swagger notation. looks like about half. I also got some updates from the Rescu maintainer and some insights how to generate rescu jaxrs compliance from swagger jaxrs-spec output.

I also maintain a lombok module from @IanWorthington and a few swagger generated modules that don't get accepted due to style guide divergence. #2436

My thinking is that the swagger generation is sufficient to get rx-java api generated where each api call is a wrapped stream-of-one synchronous request. this is theoretically a small gap to get into the xchange-stream project where the modules exist for those exchanges that already get attention. the xchange-core dto's and (tedious final class) service abstractions are used in xchange-stream as well.

the xchange-stream project does not appear to limit the approach to a single ideology or code lineage.

see https://github.com/bitrich-info/xchange-stream/issues/158 for the initial mention of this.

the likely outcome of this issue #2342 is NIH NIH NIH.

timmolter commented 6 years ago

Well after leaving this open for discussion for some time now, it looks like the majority vote for having lombok as an option, but not as a requirement. Feel free to add it and start using it then...

jnorthrup commented 6 years ago

Lombok sounds beneficial! in a sort of rescue operation for polar bears during global warming kind of way.

google has vetted kotlin, which also seems like a similar jvm compatible option for modules. kotlin has replaced java in android.

one of the outcomes of google's decision is that you can develop kotlin in jdk8 which will still compile back to jdk 1.6 for dalvik. just saying.