Consistently construct Endpoint from urls the same way#403
Consistently construct Endpoint from urls the same way#403technoweenie merged 1 commit intogit-lfs:masterfrom
Conversation
There are 2 ways to construct LFS urls - from explicit URLs and from the clone URL (which gets info/lfs appended). Previously there were multiple paths where the Endpoint could be constructed in a short-circuit manner which prevents future expansion in common functions. This standardises the Endpoint construction so it always happens via NewEndpoint, and NewEndpointFromCloneURL performs the extra step of appending info/lfs. This has a current effect of supporting SSH urls in all config settings, and not just the remote clone URL as before. But mostly this is to enable future expansion (improved SSH support)
|
Is it necessary to export the two functions? |
They're both safe for external use and conceptually robust, and Endpoint is already exported so really safe construction should be too. |
|
I don't see it as a question of “safe for external use” but as a design decision. Exported functions are the API of a package. A narrow API provides guidance on how to use a package. (Very helpful for people new to a project.) Exposed functions are also restricting programmers in terms of signature changes. Changing the signature of an exposed function, means changing the API. For now, this is not really an issue as the packages are only used internally. But in the future this might change. |
|
If that's the case then Endpoint shouldn't be exposed either. IMO you either expose the type AND the construction functions or neither. |
|
We need to export the type I considered Maybe my views are just to misguided by my OOP background. So, I'm totally open to new insights and opinions here. (Which doesn't mean that I'll give up my position without a fight. ;) ) |
|
Fair point about Endpoint() & RemoteEndpoint() although that's specific to creating from other instances. NewFoo() is the idiom in Go for publicly constructible types since you can't have protected constructors like C++/Java to tell people they shouldn't just construct themselves. I don't feel super-strong about it though, if Rick wants this internalised before accepting I'll do it. |
|
👍 on the change. I think if we want to move towards this being a package for other Go libraries, we should start moving to |
|
I'm going to merge this so it can be used in #362, unless anyone has any objections. |
There was a problem hiding this comment.
In the future I think method comments should follow standard godoc conventions.
Obviously this project isn't strict about docs. But I think if someone takes the time to write something, it should follow the conventions. It's easy enough that I may just make the change live before merging this, or applying it in a quick PR.
Consistently construct Endpoint from urls the same way
There are 2 ways to construct LFS urls - from explicit URLs and from the
clone URL (which gets info/lfs appended). Previously there were multiple
paths where the Endpoint could be constructed in a short-circuit manner
which prevents future expansion in common functions. This standardises
the Endpoint construction so it always happens via NewEndpoint, and
NewEndpointFromCloneURL performs the extra step of appending info/lfs.
This has a current effect of supporting SSH urls in all config settings,
and not just the remote clone URL as before. But mostly this is to enable
future expansion (improved SSH support)