lib-version implementation #668
Conversation
… used to compile packages, if go modules are used.
There was a problem hiding this comment.
Thanks for this, functionally it's good and the test is solid.
However some of the naming could be improved. Unless it is clear from the context we try to avoid single or dual letter variable names - especially when a comment is required to explain their intent.
| func TestParsePkgString(t *testing.T) { | ||
| type test struct { | ||
| in string // Test input | ||
| es string // Expected output |
There was a problem hiding this comment.
Can you use meaningful variable names? If it was "expectedOutput" then you wouldn't need the comment...
There was a problem hiding this comment.
So, the Fyne codebase doesn't follow Go stdlib conventions?
There was a problem hiding this comment.
I would not go so far as to say Fyne does not follow Go conventions, but I would say that the project follows the belief that if you have to have inline comments then your code could be clearer. Of course "i" is an iterator and "b" could be a buffer - where the context is clear and a convention is followed. I do not think that "es" is common shorthand for "expected output" - looking at slide#8 It would seem to indicate that "expected" could be completely inline with their recommendation.
| } | ||
|
|
||
| for _, tc := range ts { | ||
| gs, ge := parsePkgString(tc.in) |
There was a problem hiding this comment.
It's not clear from the context what gs and ge are - could they be named more clearly?
| type test struct { | ||
| in string // Test input | ||
| es string // Expected output | ||
| ee bool // Expecting error? |
There was a problem hiding this comment.
expectedError would imply an error type, perhaps shouldError is closer to the truth for a bool?
|
|
||
| func (v *versioner) doVersion() error { | ||
| c := build.Default | ||
| p, e := c.Import(pkg, ".", build.FindOnly) |
There was a problem hiding this comment.
c, p, e and ve in this method are confusing - what are they short for?
Go normally uses "err" for an error var so that's a start. Could the other variables be more clearly named?
There was a problem hiding this comment.
Perhaps in this instance build.Default could be set to "b" but I don't follow "c" as a choice. If "e" were "err" I still don't know what "p" and "ve" indicate?
|
Private feedback from the developer is that they don't want to make the requested changes. Given that code style is very important to this project the PR will be closed without merge. |
Description:
Adds a
lib-versioncommand to print out the version of fyne that would be used to compile the project.Fixes #656
Checklist:
Where applicable: