Refactor routers directory #15800
Refactor routers directory #15800
Conversation
lunny
force-pushed the
lunny:lunny/refactor_routers_directory
branch
3 times, most recently
from
94c35cf
to
5ea679c
lunny
force-pushed the
lunny:lunny/refactor_routers_directory
branch
4 times, most recently
from
901715e
to
96dd4da
lunny
force-pushed the
lunny:lunny/refactor_routers_directory
branch
3 times, most recently
from
59db1e4
to
b587910
Codecov Report
@@ Coverage Diff @@
## main #15800 +/- ##
==========================================
- Coverage 44.17% 44.14% -0.04%
==========================================
Files 684 693 +9
Lines 82289 82286 -3
==========================================
- Hits 36354 36323 -31
- Misses 40046 40074 +28
Partials 5889 5889
Continue to review full report at Codecov.
|
lunny
force-pushed the
lunny:lunny/refactor_routers_directory
branch
from
fe32ae6
to
804a500
|
@lunny you removed my changes on rebase :/ |
| ) | ||
|
|
||
| // Codes render explore code page | ||
| func Codes(ctx *context.Context) { |
zeripath
Jun 7, 2021
Contributor
Not really liking this name... I think there's some slight inconsistency with the naming of functions in this package.
Not really liking this name... I think there's some slight inconsistency with the naming of functions in this package.
lunny
Jun 8, 2021
Author
Member
The name was changed from ExploreCode to Codes which like ExploreRepos to Repos.
The name was changed from ExploreCode to Codes which like ExploreRepos to Repos.
zeripath
Jun 8, 2021
Contributor
I guess Code is already a plural - so I think the s is triggering me. Would be happier if you dropped the s I think.
Suggested change
func Codes(ctx *context.Context) {
func Code(ctx *context.Context) {
But I think there is still a structural issue with the names of page renderers and their sub components here (and elsewhere).
Within this package there are the Render* functions which admittedly have a different signature but everything else in this package is also rendering. So I'm not sure that they need the Render prefix, but I do see the point of having some distinct way of labelling things as incomplete pages. Maybe these should be suffixed Component as they're not full pages. e.g. UserSearchComponent (or moved out of the explore package so the only things it exports are actually full pages.)
If we went for the suffix approach, that would then mean that the full page renderers here & elsewhere should really gain a Page suffix - e.g. CodePage, UsersPage, OrganizationsPage and so on. However, I appreciate that that would make the web routes function a bit wordy - so maybe cleaning up the routers packages so that only full page rendering functions are ever exported from them with subpage components elsewhere might be a better idea.
With better and clearer labelling of full page rendering functions etc we'll be better placed to actually start documenting the renderers as to which paths it's expected to fire on, it's permissions, etc. and maybe even move to make routes functions be generated.
I guess Code is already a plural - so I think the s is triggering me. Would be happier if you dropped the s I think.
| func Codes(ctx *context.Context) { | |
| func Code(ctx *context.Context) { |
But I think there is still a structural issue with the names of page renderers and their sub components here (and elsewhere).
Within this package there are the Render* functions which admittedly have a different signature but everything else in this package is also rendering. So I'm not sure that they need the Render prefix, but I do see the point of having some distinct way of labelling things as incomplete pages. Maybe these should be suffixed Component as they're not full pages. e.g. UserSearchComponent (or moved out of the explore package so the only things it exports are actually full pages.)
If we went for the suffix approach, that would then mean that the full page renderers here & elsewhere should really gain a Page suffix - e.g. CodePage, UsersPage, OrganizationsPage and so on. However, I appreciate that that would make the web routes function a bit wordy - so maybe cleaning up the routers packages so that only full page rendering functions are ever exported from them with subpage components elsewhere might be a better idea.
With better and clearer labelling of full page rendering functions etc we'll be better placed to actually start documenting the renderers as to which paths it's expected to fire on, it's permissions, etc. and maybe even move to make routes functions be generated.
lunny
Jun 8, 2021
Author
Member
I will rename it to Code but not with Page suffix, otherwise all route functions will be added Page. It's awful.
I will rename it to Code but not with Page suffix, otherwise all route functions will be added Page. It's awful.
zeripath
Jun 8, 2021
Contributor
It's not great and I understand the reluctance - however there is a genuine issue here. Here are a couple of examples the problem that adding Page suffices or cleaning up the packages would help with:
in routers/repo/attachment.go:94 there is a func called GetAttachment(ctx context.Context) is this a page renderer? user.GetUserParams isn't a renderer. Turns out that it is a renderer.
RefBlame in routers/repo/blame.go is a page renderer, as is RefCommits in routers/repo/commits.go, Commits in that file is not a direct page renderer though nor are the RepoRefByType functions.
Would you guess that InitializeLabels is actually a page renderer but that RetrieveLabels is not?
From looking at a signature and name of a function you can't tell what it's supposed to be doing - and the comments aren't much better.
It's not great and I understand the reluctance - however there is a genuine issue here. Here are a couple of examples the problem that adding Page suffices or cleaning up the packages would help with:
in routers/repo/attachment.go:94 there is a func called GetAttachment(ctx context.Context) is this a page renderer? user.GetUserParams isn't a renderer. Turns out that it is a renderer.
RefBlame in routers/repo/blame.go is a page renderer, as is RefCommits in routers/repo/commits.go, Commits in that file is not a direct page renderer though nor are the RepoRefByType functions.
Would you guess that InitializeLabels is actually a page renderer but that RetrieveLabels is not?
From looking at a signature and name of a function you can't tell what it's supposed to be doing - and the comments aren't much better.
6543
Jun 8, 2021
Member
should we make the diff even more complex?
I think it's a valid issue but does it meet the scope of this pull?
should we make the diff even more complex?
I think it's a valid issue but does it meet the scope of this pull?
This reverts commit 1bbc7aa.
lunny
force-pushed the
lunny:lunny/refactor_routers_directory
branch
from
f0e0b0d
to
069fa9d
6543
merged commit 1bfb0a2
into
go-gitea:main
This PR creates
routers/web,routers/install,routers/private,routers/apifor different routers.routers/webincludes all web ui requests except api requests from the web uirouters/installincludes all install routersrouters/privateinclude all internal routersrouters/apiinclude all restful API v1 routers