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.

Sign up for GitHub

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

Jump to bottom

Remove jQuery .text() #30506

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Remove jQuery .text() #30506

wants to merge 3 commits into from
+31 −30

Conversation

silverwind
Copy link
Member

Remove and forbid .text(). Tested some, but not all functionality, but I think these are pretty safe replacements.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 15, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 15, 2024
@silverwind silverwind force-pushed the notext branch 2 times, most recently from 77ecf85 to 14bd166 Compare April 15, 2024 20:21
@silverwind silverwind added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 15, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024 •
edited

As I said in Propose to restart 1.22 release #30501 , I have objection for merging more pure refactoring (non-bug-related) PRs into main branch in recent weeks.

We should focus on making 1.22 release stable and fix regression bugs at the moment.

Just wait for some time to make the main branch stable enough, release 1.22 and start the refactoring when we do not need to heavily fix regression bugs.

delvh
delvh approved these changes Apr 16, 2024
View reviewed changes
web_src/js/features/notification.js Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 16, 2024
wxiaoguang
wxiaoguang previously requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Wait for 1.22 to be a stable release

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 16, 2024
@silverwind
Copy link
Member Author

Wait for 1.22 to be a stable release

This PR will only merge into main, it will not be backported. What is the issue?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024 •
edited

Wait for 1.22 to be a stable release

This PR will only merge into main, it will not be backported. What is the issue?

The proposal is described in #30501

image

Refactoring without full test would cause many regressions during the 1.22 release period. I believe Gitea shouldn't introduce too many regressions in a RC version.

I think we can take a look at the recent fixed regression bugs by such blindly refactoring, there are a lot, and there are still unfixed ones.

Details about recent fixes & regressions:

(and there are more .....)

There are enough regressions IMO

@wxiaoguang
Copy link
Contributor

To be clear, I never meant that I don't like refactoring, instead, I always prefer to refactor legacy code to make it more maintainable. I don't care about controllable regressions during development stage, sometimes I also introduce regressions.

I only mean that during this release period, we need to focus on making the release as stable as possible.

@silverwind
Copy link
Member Author

I still don't understand why you block this. We fork the release branch, give it 1-2 months of backports and that's generally stable enough for a release. That way, we never have to delay refactors and that has worked well in the past. No idea why you suddenly want to change this process.

@wxiaoguang
Copy link
Contributor

I still don't understand why you block this. We fork the release branch, give it 1-2 months of backports and that's generally stable enough for a release. That way, we never have to delay refactors and that has worked well in the past. No idea why you suddenly want to change this process.

I think I didn't "suddenly" change this process.

Actually, I am trying to make Gitea's release to following the history procedure. I started writing PRs since 1.15 or 1.16, from that time, I seldom saw big refactoring during the RC period. Only some are done for various necessary purposes.

But I don't understand why 1.22 and 1.23 it should start the refactoring too urgent, it would make more regressions and make backport quite difficult.

In old release, I never saw the backport became difficult.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Apr 16, 2024
@wxiaoguang
Copy link
Contributor

I dismiss my change request.

But I keep my opinion: do the best to make Gitea stable during the release, and make bugfixes easy to backport.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024 •
edited

ps: there is another open regression: on the release page, "download source code archive" causes JS error and doesn't work, I think it should also be fixed.

-> Refactor and fix archive link bug #30535

@wxiaoguang wxiaoguang mentioned this pull request Apr 17, 2024
Merged
@wxiaoguang
Copy link
Contributor

Another sample, 1.22 is broken for backporting: #30570 (comment)

@silverwind
Copy link
Member Author

Another sample, 1.22 is broken for backporting: #30570 (comment)

That's just a natural side-effect of changes. Nothing we can do about this except releasing more frequently.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 19, 2024 •
edited

Another sample, 1.22 is broken for backporting: #30570 (comment)

That's just a natural side-effect of changes. Nothing we can do about this except releasing more frequently.

It isn't natural. Because 1.22 branch is quite unstable, it is not right to backport almost everything.

There is indeed a thing could be done, I have explained above, let me explain a third time:

  1. Focus on bug fixing in recent days, and restart 1.22 release from main branch.
  2. Fix bugs for 1.22 releases (on main and backport), defer unnecessary/massive refactoring works to when 1.22.x almost gets stable enough.

@silverwind
Copy link
Member Author

silverwind commented Apr 19, 2024 •
edited

I still think we should not make such a ceremony out of releases every time. Many people run nightly because we can not decide properly when to release. Just release on a fixed schedule like every 2-3 months.

I would even go as far as saying drop the concept of release branches and release off main branch at fixed 2-3 months intervals. Would make our work a lot easier not having to worry about backports any more and such a model works for many projects, notably GitHub itself also only work off a single branch.

@silverwind
Merge branch 'main' into notext
d0303ff
@silverwind
Merge remote-tracking branch 'origin/main' into notext
453c63d 
* origin/main:
  Interpolate runs-on with variables when scheduling tasks (go-gitea#30640)
  Initial support for colorblindness-friendly themes (go-gitea#30625)
  Fix flash message for flex-container (go-gitea#30657)
  Perform Newest sort type correctly when sorting issues (go-gitea#30644)
  Fix project name wrapping, remove horizontal margin on header (go-gitea#30631)
  Add a db consistency check to remove runners that do not belong to a repository (go-gitea#30614)
  Fix wrong table name (go-gitea#30557)
  Fix compare api swagger (go-gitea#30648)
  [skip ci] Updated translations via Crowdin
  Fix queue test (go-gitea#30646)
  Enable jquery-related eslint rules that have no violations (go-gitea#30632)
  Enable more `revive` linter rules (go-gitea#30608)
  Remove obsolete CSS text classes (go-gitea#30576)
  Hide diff stats on empty PRs (go-gitea#30629)
  [skip ci] Updated licenses and gitignores
  Use correct hash for "git update-index" (go-gitea#30626)
  Fix repo home UI when there is no repo description (go-gitea#30552)
  Fix dropdown text ellipsis (go-gitea#30628)
  fix(api): refactor branch and tag existence checks (go-gitea#30618)
  Add --skip-db option to dump command (go-gitea#30613)
wxiaoguang
wxiaoguang reviewed Apr 23, 2024
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I think there is no need to request review from me, I could approve it some days later, but not now.

I have said clearly, many times:

  1. We need to make 1.22 release as stable as possible.
  2. I can't accept a "partially tested PR" merged during RC period.
  3. I do not see why this PR should be so urgent, it is a pure refactoring PR, no bug fix.

@silverwind
Copy link
Member Author

Not urgent, but I'd like to reduce my open PR count.

@silverwind silverwind marked this pull request as draft April 25, 2024 08:16
@silverwind
Copy link
Member Author

I will give the affected parts some testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/internal modifies/js size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@silverwind @wxiaoguang @delvh @GiteaBot