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.
