You are using interfaces to define a type. The overhead of interface isn’t
necessary to achieve the same goal. I see that github.com/gorilla also uses
this pattern in places. Supporting another tool or providing a mock for
testing is just making a new value of the type.
I do use sub-packages and encapsulation to simplify working with certain
concepts, but here my opinion is the use of encapsulation is not adding
much value. My experience has been that Go utility functions are more
easily copy-pasted into each project rather than shared, although for an
ecosystem of applications I can see that shared packages would form. But
for this project the effort seems too early. My opinion is make the package
when you need it.
Matt
On Wednesday, January 24, 2018 at 10:06:17 AM UTC-6, Clinton Begin wrote:
>
>
> Hi Matt, thanks for the review.
>
> Here is some feedback and explanation.
>
> >> Does BuildTool need to be an interface? Having methods on a pointer to
> nothing (type GoBuildTool struct{}) seems unnecessary to me.
>
> Interfaces are expressions of functional contract, and have little
> dependency or basis for requiring state (fields on a struct). BuildTool is
> an interface for two reasons. The first is that there is potential to
> support other build tools other than the go executable. I briefly
> considered supporting gb for example. But also, who knows. Someday maybe
> there will be a gnu go compiler or some such thing. The second reason is
> unit testing. While I don't mock that interface in any of the current unit
> tests, this allows me to if I need to at some point. In some languages
> performing an extract interface refactoring after the fact is easy. Golang
> is not one of those languages. So erring slightly on the side of interfaces
> is worth it, especially where there are clear cases for alternative
> implementations or test isolation from external dependencies (both
> applicable in this case).
>
> >> Also, why not have this at the level of package main?
> >> Same with graven/commands, graven/config, graven/domain, graven/util,
> since these are specific to
> >> graven they could also be part of package main. Config is just defining
> one type.
>
> As a practice I try to keep as much out of main as possible. Not only does
> it keep code more organized as the project grows, as in any other language,
> but in Go it also means that I can access this code as a library from other
> potential tools or test harnesses. I don't let the number of files dictate
> what should be a package or not. It's functional context that contributes
> to that decision.
>
> >> interface makes sense when just grouping behavior with no data
> structure
>
> Again, interfaces have nothing to do with state or data. An interface is a
> functional contract. It says: Any such thing that deems itself as
> implementing an interface (implicit in Go), must implement these functions.
> There is no reliance or direct relationship to state, either by design,
> contract or even pattern. In fact, more often than not, interfaces describe
> the core logic of a system or interfaces to external systems, and having
> mutable state in such implementations leads to a source of bugs in the form
> of shared state, race conditions and locking. For this reason, many
> interfaces choose to accept their state as parameters to their functions,
> rather than store them as part of the implementation.
>
> I recommend this blog post to learn more:
> http://jordanorelli.com/post/32665860244/how-to-use-interfaces-in-go
>
> >> Well-named package main files and types provide just as good logical
> separation.
>
> I disagree here. Golang has very weak encapsulation. There are very few
> options for protecting access to fields and functions. Go only has package
> level access rules, and therefore packages do a lot more for accessibility
> than is necessary to consider in other languages. In C# or Java, sure, you
> could put every file in one package or directory and protect access to
> fields and functions. But you still wouldn't do that for the sake of code
> organization and even in Java, scoping as well (protected is also package
> scoped). So I disagree with your suggestion here in Go and any other
> language really.
>
> >> graven/resources/commmon is misspelled. I’m not sure why you have these
> simple text files when a const in code
>
> Well that is embarrassing. :-) Those are old test fixtures that I don't
> think are used anymore. I've since moved test fixtures to the test_fixtures
> directory, but those seem to have been left behind. Thanks for catching
> that.
>
> Cheers,
> Clinton
>
> On Sunday, January 21, 2018 at 9:36:57 AM UTC-7, matthe...@gmail.com
> wrote:
>>
>> Hi Clinton, here’s a code review.
>>
>> Does BuildTool need to be an interface? Having methods on a pointer to
>> nothing (type GoBuildTool struct{}) seems unnecessary to me. Also, why not
>> have this at the level of package main?
>>
>> Same with graven/commands, graven/config, graven/domain, graven/util,
>> since these are specific to graven they could also be part of package main.
>> Config is just defining one type.
>>
>> Since graven/domain.Project, Artifact, Repository, Version only has read
>> operations defined why not have the struct as method receiver instead of
>> pointer to the struct? I’d only move to the pointer for performance if
>> proven by a testing benchmark, pprof profile, and need, otherwise no
>> pointer is more understandable/readable.
>>
>> Again in graven/repotool, graven/vcstool, I’m not sure having an
>> interface makes sense when just grouping behavior with no data structure.
>> Defining a struct type with function fields with a var for docker, github,
>> and maven may be better. These packages are also application specific;
>> packages should be portable and not just used to group application logic.
>> Well-named package main files and types provide just as good logical
>> separation.
>>
>> graven/resources/commmon is misspelled. I’m not sure why you have these
>> simple text files when a const in code could maybe provide the same
>> functionality.
>>
>> Matt
>>
>> On Saturday, January 20, 2018 at 10:54:29 PM UTC-6, Clinton Begin wrote:
>>>
>>> Hi all,
>>>
>>> I'm looking for feedback on a build tool I created to fill some gaps I
>>> saw in the Golang project build space. While I've used this tool for 9
>>> months or so, you can very much consider it a prototype of sorts, or a
>>> concept, and PRs are definitely welcome. This is hopefully the beginning of
>>> a conversation, not an assertion that this is a finished product.
>>>
>>> https://github.com/cbegin/graven
>>>
>>> There's an intro video and short design motivation doc. Remember that
>>> motivation works both ways. It influences both things we want to emulate,
>>> and things we want to avoid. So please don't be put off immediately by the
>>> Maven references. While Maven as a tool can be a nightmare, as a concept,
>>> it's purpose, goals and what it's proven in the Java space, are worth
>>> learning from.
>>>
>>> Cheers,
>>> Clinton
>>>
>>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
