search

travelgateX / go-jwt-tools

Golang authorization middleware for JWT tokens. JWT tools
http://www.travelgatex.com
GNU General Public License v3.0
9 stars 9 forks source link

Unify the parse and cache sub-package functionality #19

Open guzmanthegood opened 5 years ago

guzmanthegood commented 5 years ago

It's a bad practice to have different sub-packages in a common-use repository.

Dependency managers (such as the dep) only understand the repository as a single package. https://github.com/golang/dep/issues/1208

You have to unify this functionality, my recommendation is to create a file parser.go and another cache.go and group that functionality in there, for a package as small as jwt-tools it is not necessary to have 3 packages.

@zechao & @pperaltaisern , pls

pperaltaisern commented 5 years ago

Ok I understand now that dep relies on a repository and not single packages. However the subpackages here are meant to put a layer of abstraction between functionality (the root package) and the implementation details (jwt and cache, by now) and they all should always be at the same version.

In that matter it looks fine to me to have all the packages in this repository sharing the same version of dependency, since it has no sense to import them with separate versions, so I don't see any issue here.

I followed Ben Johnson guidelines on how to organize code: https://medium.com/@benbjohnson/standard-package-layout-7cdbc8391fc1

If you still think that this is a bad practice, I think we should discuss it with the rest of our go community.

guzmanthegood commented 5 years ago

Hi @pperaltaisern , I think I have mixed several problems and I agree that we can discuss it in the community.

I see what you said the other day about the versions, there was an error and the correction of it wasn't uploaded to version 1.2.0. I think for this specific problem it's best to add a new version, for example 1.2.1 and remove 1.2.0. But don't know really.

About the structuring of the code, I share with you that you are respecting good practices (I had also read the excellent article by Ben) and it is possible that in the tests I was doing with "dep" I was confused with:

  1. not really know how is the new use of the library (with the inclusion of jwt and cache modules)
  2. the commented bug that causes the wrong library to be imported (1.2.0)

PS: Ben also suggests that for packages of less than 10,000 lines of code it can be valid to group it into a single package. But like everything, it is debatable.

So I think its better to have a meeting to discuss this points.

Thx for your time.

EDIT: @pperaltaisern we just added 1.2.1 version fixing the bug