-
Notifications
You must be signed in to change notification settings - Fork 5
golang package and test best practice #21
Comments
Regarding tests: this suggestion was previously discussed, and I recall @spy16 was opposed to moving tests into the same package as code. IIRC, his position was that placing tests in a separate package forces developers to test the behavior of the high-level API rather than testing implementation details. The rationale is that this decoupling of tests from implementation is more robust to changes in the codebase. Personally, I'm not opposed to moving tests into the Regarding the standard go layout: I'm not opposed to this as I make heavy use of To reiterate: I'm mostly onboard with the proposed changes, but just want to make sure I understand both the problem and the solution. The package layout has been an annoyance in the past, so this may well be a good time to fix it. As usual, thanks for your contributions! |
The basic problem is that if I want to do a PR I have to modify a bunch of files to replace the temporary package name instead of just keeping my own copy of go.mod |
I'm not sure I see where temporary packages or changes to
Am I overlooking something? I realize it can be tedious to explain/reproduce these issues, but this kind of feedback is really precious -- it helps us ensure we support as many workflows as possible. Overall, this is a sensible proposal and it has my support, pending @spy16's input regarding the |
Yeah all the tests depend on the spy16 version so I'm not actually testing my code with that approach afiu |
I thought this problem was solved with go mod since go tools are now looking at the |
I don't see how. Remember that I need to be able to test and use leifj/sabre as a dependency of other projects while we talk about PRs etc. So I clone my fork of sabre and make changes. I type make - go build identifies spy16/sabre as a dependency of leifj/sabre via the tests. However this may all be a moot point if it turns out 'case' is just a macro. |
Aha! This was the missing piece! 😃
In this particular case, yes, but I still think this is a problem worth fixing. @spy16 I can relate to your reasons for wanting the tests to live in a separate package. What do you think of doing the following?
I realize it's a bit less clean than enforcing this through separate packages, but it may be a good compromise for the sake of users in @leifj 's position. |
While I agree the issue is real and there should be a way to solve this (and there is; see below), I do not agree that using same package is considered best practice (I do make exceptions to this rule, for example
Also, Coming back to the issue for forked versions: I would be okay to go with approach proposed by @lthibault if it solves the issue, but it doesn't completely since the import path issue still remains for There are 2 cases where users would want to fork:
The issue of working with a 3rd project by importing the forked version will still remain with the 2 remote approach mentioned above. But I think it is a bad idea to work on a 3rd project with the assumption that the changes will be merged upstream: PR discussion etc are happening precisely to discuss whether they should be merged to upstream or not. May be a different design is expected by maintainers or may be PR gets rejected. What happens to the project depending on the fork then?. Also, it is quite possible that the changes done on forked version get heavily influenced by that 3rd project as well.. But, In worst case if this is still needed (may be maintainers are taking too long to respond and you don't want to get blocked with your project), you can make use of Let me know if this works. 🙂 |
I have been working on an implementation of the 'case' construct and the package layout including how tests are maintained is causing a bit of pain. I suggest moving the project to a standard golang layout (placing packages in 'pkg') with tests in the same package as the code. As it is now I have to modify every file to create a PR that adds a single function.
The text was updated successfully, but these errors were encountered: