abcweb
abcweb copied to clipboard
Remove `abc` prefix from the subpackages
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 :-).
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?
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.
- https://github.com/volatiletech/abcweb/blob/master/templates/app/setup.go.tmpl#L15
A cursory look in the templates
subpackage seems to show that this is happening only twice for:
- chi
- render
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. :-)
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.
Todo: move abcrender interface to abcweb, so dependency on abcrender can be eliminated if using a different renderer instead of unrolled/render.