rename some task packages #770
Conversation
This has a few follow-on effects. Namely one no longer needs to have a special name for the task/test package (previously this was custom named to "taskTest"). Now it just follows naturally as "tasktest" with no custom naming required. This change brings the name inline with current Go best practices for package names and package naming, and also makes it easier for LSP-enabled editors to automatically manage Go imports. In addition, it follows patterns found in the Go stdlib (see httptest, slogtest, fstest, iotest, etc.) I also renamed task/test/mock.go to task/tasktest/taskclient_mock_gen.go just to make it extra clear that the file is generated, as well as what it contains (not just a mock, but a mock of the task client). [^1]: https://go.dev/doc/effective_go#package-names
This has a few follow-on effects. Namely one no longer needs to have a special name for the task/store/test package (previously this was custom named to "taskStoreTest"). Now it just follows naturally as "taskstoretest" with no custom naming required. This change brings the name inline with current Go best practices for package names and package naming[^1], and also makes it easier for LSP-enabled editors to automatically manage Go imports. In addition, it follows common patterns from the Go stdlib (see httptest, slogtest, fstest, iotest, etc.) In general I would have preferred to keep the name to the concatenation of only two package names, but "store" is a very common package within platform, so I think it makes sense to keep all three parts, as otherwise there would be eight "storetest" packages[^2] and we'd be back to custom renaming the imports which just makes things harder all around. [^1]: https://go.dev/doc/effective_go#package-names [^2]: Yes, eight. To see for yourself: Run `$ go list ./... | rg '/store/' | sed -e 's,/store/.*,/store/,' | sort | uniq`.
This has a few follow-on effects. Namely one no longer needs to have a special name for the task/service/test package (previously this was custom named to "taskServiceTest"). Now it just follows naturally as "taskservicetest" with no custom naming required. This change brings the name inline with current Go best practices for package names and package naming, and also makes it easier for LSP-enabled editors to automatically manage Go imports. In addition, it follows patterns found in the Go stdlib (see httptest, slogtest, fstest, iotest, etc.) In general I would have preferred to keep the name to the concatenation of only two package names, but "service" is a very common package within platform, so I think it makes sense to keep all three parts, as otherwise there would be four "servicetest" packages[^2] and we'd be back to custom renaming the imports which just makes things harder all around. [^1]: https://go.dev/doc/effective_go#package-names [^2]: `$ go list ./... | rg '/(task)?service/?test'`
ewollesen
force-pushed
the
eric-task-tweaks
branch
from
40070cc to
710b948
Compare
There was a problem hiding this comment.
Hmmm... What actual problem is this solving?
Also, I don't recall a documented best practice about naming test-only packages. Can you point me to that?
Also, while platform may not conform to other current best practices, I don't see the point of make changes unless it has a functional reason. Plus, these type of isolated changes (i.e. just in these few locations) just make the whole repo inconsistent and very difficult for someone to know which is the standard way to do so for this repo.
If your primary concern is that you have to explicitly name the import, then just look at the rest of the platform imports for platform "standard" of using explicit import names to make it very clear what is being used.
Changes don't have to solve a problem to be an improvement. Sometimes when I see a place that I think an improvement can be made, I try to make the improvement, in the hopes that it will make things better. I recognize these are subjective, and you might not agree.
I don't believe I claimed such a thing exists. I see the pattern in the stdlib, and I think it's a good, clear, pattern, so I duplicated it, and mentioned that in my commit messages to indicate it was the "inspiration" if you will for the naming.
I'm just coming from a place of trying to show an example of how I think the code could be improved. You too have suggested changes to code to improve its readability. Those aren't functional reasons, but they still matter, right? Making this kind of change throughout the entire code base all at once will be difficult, and not a great use of time, but not improving things where we can, means stagnation. Consistency is a useful feature, but not one worth preventing progress or improvement, IMO.
When I see explicit names, I'm more likely to be confused: "github.com/tidepool-org/platform/service" is imported bare, or as "platform" or as "router". "github.com/tidepool-org/platform/service/service" is imported bare, or as "serviceService". The import "serviceTest" could refer to any of "github.com/tidepool-org/platform/auth/service/test", "github.com/tidepool-org/platform/prescription/service/test", or "github.com/tidepool-org/platform/service/test". Some of those are also imported in a bare fashion. Those were the first few places I looked. There's not much existing consistency that I can see, and when a name is explicitly named, that name is often neither clear, nor consistent. My point being, I don't think consistency is a good argument not to make some changes. ...and I'm not suggesting that we never explicitly name an import, but I'm suggesting that the changes in this PR can reduce some amount of confusion and improve the current situation. I'm a fan of small incremental improvements. Things grow and evolve and change over time, and I think that's a good thing. |
|
Per our offline discussion, I'm good with the idea. Please propose the issue and solution to the team for general discussion. |
See the individual commit messages for the details. I'm open to feedback if this causes anyone stress.