tendermint / coding

14 stars 10 forks source link

What are the benefits of a types package? #61

Open xla opened 6 years ago

xla commented 6 years ago

Personally I feel very strongly about types and would go as far as call it an anti-pattern. Happy to go into detail why, but before curious to hear good arguments for the advocate pratice.

Reference in the tendermint codebase: https://github.com/tendermint/tendermint/tree/master/types

liamsi commented 6 years ago

I'm not sure I understand what you mean by types. Do you mean a package like this: https://github.com/tendermint/tendermint/tree/master/types ? If not, could you provide an example?

xla commented 6 years ago

@Liamsi Exactly that, sorry for failing to provide proper reference, amended

rigelrozanski commented 6 years ago

Eager to hear your strong opinions/ alternative designs to the types package. I think in the context of the SDK - types is nice explicitly for when trying to reference an interface. If we limit the implementations of the types to outside of the types package I'd imagine it's a maintainable and safe development pattern.

I imagine that clumping to much into a single package could raise issues down the road (thoughts here?) - but also I to much package (and file) separation can be a real hassle to work with and create obfuscated code bases which are difficult to navigate and understand - which is a real issue.

melekes commented 6 years ago

https://medium.com/@benbjohnson/standard-package-layout-7cdbc8391fc1

xla commented 6 years ago

@melekes The article doesn't mention a types package, rather talkes about a root package and sub-packages.

rigelrozanski commented 6 years ago

Yeah typically the design pattern I've seen is where folks use the root directory kind of like a types package, What's your opinion on this @xla vs straight up types/ directory?

xla commented 6 years ago

IMHO there is only one valid application of the a package called types and that is the one that standard lib offers: https://golang.org/pkg/go/types/ as it concerns itself with the language native concept of a type.

Resorting to something like types seems as weak as a package called util, ultimately doomed to be a bucket where anything is dumped into as it seems too hard at the time to figure out where it actually belongs. Packages are encoded boundaries of the domain and most likely your domain would not be concerned with types. Furthermore will it open up this massive namespace which adds additional bookkeeping overhead for the people working on the codebase:

types.FooBar
types.FooBaz
///
foo.Bar
foo.Baz

My point is that when using something as generic as types you take away an important dimension of code organisation and communication of intend.

Eager to hear your strong opinions/ alternative designs to the types package. I think in the context of the SDK - types is nice explicitly for when trying to reference an interface. If we limit the implementations of the types to outside of the types package I'd imagine it's a maintainable and safe development pattern.

In reality types seldomly stand in isolation and are almost always bound with functionality, which end up in the same package, which leads to that code needed to be tested. If we assume that the Dependency inversion principle has any value then ideally interfaces should be declared where they are needed, while there are strong examples of widely used interfaces, e.g. io.Reader, their surface is so small that a package depending on it could easily declare it in place. Which is a desirable characteristic anyway to ensure that the scope of an interface doesn't blow up.

liamsi commented 6 years ago

I don't feel so strong about the types that I would call it an anti-pattern. But I do agree with every concern @xla raises above. As someone who is new to the code base, I found the name of the package confusing, as I was expecting something related to an internal type system. It's difficult to guess from the package name what the structs in types are used for.

As far as I understand, the structs there are the ones which will be send around via JSON / amino? Is that correct? If so, why no call it messages (or aminos) and move everything which isn't send around into other packages?

rigelrozanski commented 6 years ago

@Liamsi lol we need need to name something "aminos" way to amazing

rigelrozanski commented 6 years ago

Disclaimer: Cosmos-SDK Centric CC: @sunnya97

@xla great point about the additional namespace overhead - totally with you on that one. Not sure what this looks like in tendermint but in the SDK there actually ARE "types that stand in isolation of functionality". In this situation the types are defined in the sdk and utilized by independent modules (with the intention of being able to mix/match swap out modules) - So in this particular instance - I actually feel that it makes sense to have these high level type definitions (intended to be fulfilled by arbitrary modules) separated from their functionality. For everything else that is not explicitly what I just mentioned I whole-heartedly agree that it doesn't belong in the types/ -> and this does currently exist. I'm thinking that all these pieces of functionality (typically small amounts of logic) should exist in unique packages maybe clumped together in a common folder aka common/yourpackage/

What are your thoughts on that?

jaekwon commented 6 years ago

The "types" folder is there in place of putting all the files in the root directory. Keeping project-wide common types esp for external callers in the root directory is the standard convention for Golang. We want to keep the root directory clean, Makefile and Godep.toml etc., so moving every go file from the root directory to a special directory is IMO an improvement over existing Golang conventions.

"util" is a catchall for internal implementations and it does get unwieldy. But the SDK itself IMO should have a catch-all package called "sdk" where a lot of common interfaces/structs live.

Handler, AnteHandler, FeeHandler, Context, Address, Account, AccountMapper, Coin/s, Error, StdSignature, Tag/s, Tx, Msg, StdSignDoc, and all the store interfaces, I believe they should be referred to as "sdk.*" so we can standardize around these common interfaces and structures. They're all common elements that one needs to know about to fully grasp the SDK, as the SDK is built today.

It's true that others can build alternative systems using the components we've already built... like a ParallelBaseApp (Sunny's suggestion). But that shouldn't prevent us the SDK having specific opinions today that get someone started on building a kickass app. Most people don't need parallel tx processing. The types defined in "types" and the opinions that it represents, should be sufficient for the majority of uses-cases.

BaseApp should be the structure that users embed in their CustomApp. So BaseApp should embed IAVLApp, that seems fine. IAVLApp can live in cosmos-sdk/iavlapp or something. So we can have /baseapp and /iavlapp.

rigelrozanski commented 6 years ago

cool thx

xla commented 6 years ago

The "types" folder is there in place of putting all the files in the root directory. Keeping project-wide common types esp for external callers in the root directory is the standard convention for Golang. We want to keep the root directory clean, Makefile and Godep.toml etc., so moving every go file from the root directory to a special directory is IMO an improvement over existing Golang conventions.

@jaekwon Could you reference where this convention orignates from? Personally I've never seen this to be the case and even light inspection of popular Go projects shows a total different approach. If anything people are advocating for package division by domain, What are the main concepts, where do they interface, etc. And then have their types close to them and not remote in another place. To give some context the CodeReviewComments explicitely state to avoid meaningless package names.

Some more thoughts on packages:

As a rule of thumb, keep types closer to where they are used. This makes it easy for any maintainer (not just the original author) to find a type. In go, package names are not plural.

In the end I don't feel strongly about this issue because I want to see conventions, best-practices or the soup du jour being followed, but I strongly believe that this has real implications on the quality and complexity of the software we write. As we would benefit from the constant conversation how something fits into the domain and what it exposes to the world.