volatiletech / abcweb

Go web app framework and generator. Inspired by Rails.
BSD 3-Clause "New" or "Revised" License
265 stars 15 forks source link

Remove `abc` prefix from the subpackages #6

Open yml opened 7 years ago

yml commented 7 years ago

This is the second time that I stumble on this project. I sent you a PR few days ago. There is something that annoyed me in your API. Some of your packages are prefix by ABC for no obvious reason. I think that removing this prefix will increase the readability and reduce the typing.

This is close from bikeshedding so feel free to ignore it and just close this issue :-).

nullbio commented 7 years ago

Hi yml. I understand what you're saying. I've put that prefix so that there are no naming collisions because those package names are pretty common, for example "database", "config", "middleware", "render", these have easy conflicts and I didn't like the idea of taking these package names away from other packages or from the users. For example, "render" is already reserved by this: https://github.com/unrolled/render -- which is something we use in abcweb. "middleware" is also reserved by the Chi router: https://github.com/go-chi/chi/tree/master/middleware which is another thing we use. I do agree with you that it's annoying though. I'll give it some more thought on how we can make this better. Do you have any ideas/thoughts on this?

yml commented 7 years ago

I guessed that was the reason nevertheless it is only a problem if I want to import both of them on the same file. It is interesting to see that when this problem happen in the templates you prefix the external lib by its package name.

A cursory look in the templates subpackage seems to show that this is happening only twice for:

It might be just me but reading the doc on godoc It is not immediately obvious from where render.Render is coming from. The fact that your Render is embedding a *render.Render is an implementation detail. If you better encapsulate the New to not expose the render.Options you might avoid to leak it.

Another approach that is not exclusive with the one above is to flatten the structure of abcweb and to move some of exported subtype directly under abcweb. The api will become abcweb.NewRender(...).

Thank you for taking the time to consider my feedback instead of just closing the ticket. :-)

nullbio commented 7 years ago

In regards to your Render suggestion, you definitely have a valid point here. Could you please clarify that this is the solution you had in mind: Unexport the Render struct in abcrender package. Create an Options struct in abcrender that has the same fields as the unrolled render.Options struct, and pass these values along in the New method to the unrolled render options struct so that it's not exposed to the user. If people wish to create or use alternative renderers they can implement the abcrender.Renderer interface and create their own New method. This will be a breaking change.

After giving it some more consideration I'm not particularly fond of renaming the packages. As you've pointed out, prefixes can be used to rename packages if desired, so if it's annoying people they can always prefix on import. The abc prefix is also rather short, but I do understand what you're saying about it being annoying, I've experienced that myself. So, I think your suggestion about flattening the structure and moving to an exported subtype system directly under abcweb is the best option. This would also make it easier to write documentation for all of the features and have it all centralized instead of having to send people to all different packages. This is however another breaking change and is quite a lot of work to do for all abc packages (which I would like to do for consistency).

Due to the fact that both of these are breaking changes I would like to push this back to v3 and collect further feature requests and bug reports in the mean time before a release, so that we don't have to bump the version to v4 in a really short timeframe. I've marked this issue as v3.

nullbio commented 4 years ago

Todo: move abcrender interface to abcweb, so dependency on abcrender can be eliminated if using a different renderer instead of unrolled/render.