Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ctx in templates #23105

Merged
merged 10 commits into from
Mar 2, 2023
Merged

Refactor ctx in templates #23105

merged 10 commits into from
Mar 2, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Feb 24, 2023

Before, the dict "ctx" ... map is used to pass data between templates.

Now, more and more templates need to use real Go context:

ctx is a Go concept for Context, misusing it may cause problems, and it makes it difficult to review or refactor.

This PR contains 2 major changes:

  • In the top scope of a template, the $ is the same as the ., so the old labels_sidebar's root is the ctx. So this ctx could just be removed. bd7f218
  • Rename all other ctx to ctxData, and it perfectly matches how it comes from backend: "ctxData": ctx.Data. 7c01260

From now on, there is no ctx in templates. There are only:

  • ctxData for passing data
  • Context for Go context

@wxiaoguang wxiaoguang changed the title Clarify ctx in templates Refactor ctx in templates Feb 24, 2023
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 24, 2023
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 26, 2023
@yardenshoham yardenshoham added this to the 1.20.0 milestone Feb 27, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 28, 2023
@techknowlogick techknowlogick added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Feb 28, 2023
@techknowlogick
Copy link
Member

Marking this as breaking, normally we don't for custom templates, but marking as breaking ensures it'll be mentioned in the blog post.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 28, 2023

I do not think Gitea has promised the template compatibility between releases. The "custom" document says that users should always get matched templates to customize.

If the template changes are considered to be breaking, there will be more but not only:

ps: I have no objection for marking this as breaking, as long as if helps changelog/blog.

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we do about the still present PullRequestCtx?

@wxiaoguang
Copy link
Contributor Author

What do we do about the still present PullRequestCtx?

Good point .... I didn't find it.

IMO, luckily, the PullRequestCtx is not like ctx or Context, so maybe it's fine to keep it, or rename it in the future.

@lunny
Copy link
Member

lunny commented Feb 28, 2023

I do think Gitea has promised the template compatibility between releases. The "custom" document says that users should always get matched templates to customize.

If the template changes are considered to be breaking, there will be more but not only:

* [Move helpers to be prefixed with `gt-` #22879](https://github.com/go-gitea/gitea/pull/22879)

* [Refactor hiding-methods, remove jQuery show/hide, remove `.hide` class, remove inline style=display:none #22950](https://github.com/go-gitea/gitea/pull/22950)

* ....

ps: I have no objection for marking this as breaking, as long as if helps changelog/blog.

I would like to mark it so that users could know which templates have been changed. And I also think we could put some standards about when should break be marked in some documentation, so that maintainers could keep consistent behaviors.

@codecov-commenter
Copy link

Codecov Report

Merging #23105 (bf624ca) into main (7a5af25) will not change coverage.
The diff coverage is 66.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main   #23105   +/-   ##
=======================================
  Coverage   47.55%   47.55%           
=======================================
  Files        1140     1140           
  Lines      151032   151032           
=======================================
  Hits        71816    71816           
- Misses      70712    70714    +2     
+ Partials     8504     8502    -2     
Impacted Files Coverage Δ
routers/web/repo/issue.go 36.61% <66.66%> (ø)
modules/queue/queue_bytefifo.go 59.20% <0.00%> (-1.81%) ⬇️
modules/queue/unique_queue_disk_channel.go 64.73% <0.00%> (-1.34%) ⬇️
routers/api/packages/nuget/api_v3.go 97.24% <0.00%> (+2.75%) ⬆️
modules/charset/charset.go 75.36% <0.00%> (+3.62%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 2, 2023
@jolheiser
Copy link
Member

🎺 🤖

@jolheiser jolheiser merged commit ce73492 into go-gitea:main Mar 2, 2023
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 2, 2023
@wxiaoguang wxiaoguang deleted the fix-tmpl-ctx branch March 2, 2023 17:48
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 3, 2023
* giteaofficial/main:
  Use async await to fix empty quote reply at first time (go-gitea#23168)
  Fix switched citation format (go-gitea#23250)
  Improve update-locales script and fix locale processing bug (go-gitea#23240)
  Refactor `ctx` in templates (go-gitea#23105)
  Improve frontend guideline (go-gitea#23252)
  Close the temp file when dumping database to make the temp file can be deleted on Windows (go-gitea#23249)

# Conflicts:
#	templates/repo/issue/view_content/context_menu.tmpl
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
@delvh
Copy link
Member

delvh commented Jun 26, 2023

Marking this as breaking, normally we don't for custom templates, but marking as breaking ensures it'll be mentioned in the blog post.

@techknowlogick ?

@delvh
Copy link
Member

delvh commented Jun 28, 2023

@techknowlogick second ping.
Why should we mention it in the blog post unlike all other template changes?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants