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

Types Folder as an Alias Hub, not a types hub #1123

Closed
rigelrozanski opened this issue Jun 2, 2018 · 5 comments
Closed

Types Folder as an Alias Hub, not a types hub #1123

rigelrozanski opened this issue Jun 2, 2018 · 5 comments
Assignees
Labels
C:baseapp Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.

Comments

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Jun 2, 2018

Rather than having the types directory be a mish mash of all the possible types which are defined in other packages (typically either root sdk packages or x/ packages) types should be reorganized in the following way:

  • if the type is intended to be defined from modules in x/ (can't be statically depended on) in keep them as in the types folder
  • if the type is defined elsewhere in the sdk but we just wanted to be easily accessible to the to anybody importing the sdk (so we defined it in types so they could just reference the one folder) move the definition of that type to the (non-x/) package where it's implementations functionality is defined, and then create and alias to it in the types/ directory. This way as a new user you can still simply import types/- however the definition of the type is now grouped with it's implementation (neater)
  • if the type (in types/) has substantial associated logic (such as rational IMO) right there in the types directory a good practice could be to move it to a new package, maybe as a subpackage in types, and then alias it just like the previous bullet.

CC @jaekwon @xla @ebuchman @cwgoes @sunnya97

EDIT: Another consideration - "loose" helper functions sitting in types such as PrefixEndBytes in types/store.go should be moved to their categorical package and then referenced with a function alias - this particular example:

  • types/store.go/PrefixEndBytes moves to store/helpers.go/PrefixEndBytes
  • types/store.go adds:
    • var PrefixEndBytes = store.PrefixEndBytes

kapoosh!

@xla
Copy link

xla commented Jun 2, 2018

if the type is defined elsewhere in the sdk but we just wanted to be easily accessible to the to anybody importing the sdk (so we defined it in types so they could just reference the one folder) move the definition of that type to the (non-x/) package where it's implementations functionality is defined, and then create and alias to it in the types/ directory. This way as a new user you can still simply import types/- however the definition of the type is now grouped with it's implementation (neater)

Is the sole motivation reduction of import statements for integrators? Referencing the Go Code Review Comments on package names here. There was an internal discussion on this before, just to surface it again, what are the requirements and needs from integrators (how many consumers of the SDK exists) we optimise for here?

@rigelrozanski
Copy link
Contributor Author

Is the sole motivation reduction of import statements for integrators?

Yeah a large motivation of the types packages is to make it dead-simple straight forward to import any public type - shouldn't have to think about this as a developer I don't think.

Referencing the Go Code Review Comments on package names here

Yeah I'm also aware of the merit of limiting the variable definition length through the use of separated packages (more imports) as the link points out - it's really just a tradeoff with number of imports. But yeah also it's not a huge issue for the sdk, browsing through the types packages it's mostly just the NewFooBar functions which are longer than they would need to be.

@rigelrozanski
Copy link
Contributor Author

rigelrozanski commented Jun 5, 2018

summarizing a discussion with @xla

There are really three elements which need to be considered:

  • SDK (./ not incl /x, /cmd, /examples)
  • Modules (/x) consumers of: SDK
  • Applications (/cmd, /examples) consumers of: SDK, modules

we came to the conclusion that we should:

  • Have the aliases on the root of cosmos-sdk not in the types package - this will effectively act as the types package used to
  • The root package name should be cosmos to bring some context for third party packages importing the cosmos-sdk repo
  • consumers of the SDK (aka modules/applications) will import only from the aliases for ease
  • internally with the SDK the aliases are not used, and the specific sub-packages should be referenced

@jackzampolin jackzampolin added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. and removed core labels Oct 12, 2018
@rigelrozanski
Copy link
Contributor Author

So we can actually keep the repo-name as Cosmos-Sdk and just name the package cosmos and then imports will accept the cosmos as the import by default (even without an alias in the import)

@rigelrozanski rigelrozanski self-assigned this Feb 19, 2019
@rigelrozanski
Copy link
Contributor Author

After having a convo with @jaekwon about this issue - we decided that this is a minor priority for the time being, however there are a couple offshoot issues #3928 #3929

It would be really great if we had the opportunity to import from the repo level without requiring a giant alias file... I don't think go allows this though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:baseapp Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

No branches or pull requests

5 participants