Skip to content

Conversation

techknowlogick
Copy link
Member

No description provided.

@techknowlogick techknowlogick added type/docs This PR mainly updates/creates documentation skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Sep 4, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 4, 2025
@techknowlogick techknowlogick enabled auto-merge (squash) September 4, 2025 15:13
@wxiaoguang
Copy link
Contributor

  1. Who are using all these labels and how they are used? For example: this PR has modifies/internal, is such label used somewhere?
  2. tools/package.json isn't really related to Gitea's dependency?

@techknowlogick
Copy link
Member Author

I use the labels as a signal about what might be included in the PR, and what changes are made. Also, it helps detect changes to PRs that might be unrelated to the PR scope.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 4, 2025

TBH some labels seem noise to me, for example: "api" (a lot of unrelated), "cli" (a lot of unrelated), "internal" (unclear what it is for), "go" (most PRs change go files), "frontend" (tmpl also has frontend layout)

And why this PR has "docs" label.

@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 Sep 4, 2025
@denyskon
Copy link
Member

denyskon commented Sep 4, 2025

I find the system really useful, it allows to filter PRs when reviewing and also to detect a scope of a PRs, for exmple how many different sections they change. It's a difference in my review approach if I see am small CSS change or a PR which modifies go, templates and assets.

@denyskon
Copy link
Member

denyskon commented Sep 4, 2025

Of for example if I see a PR with modifies/migration I know directly that oops I shouldn't be approving it because I'm not that experienced with migrations

@wxiaoguang
Copy link
Contributor

Of for example if I see a PR with modifies/migration I know directly that oops I shouldn't be approving it because I'm not that experienced with migrations

Then why this PR has "docs" label?

Why "tools" directory affects Gitea's dependencies? None of these libraries in "tools" directory would go into Gitea's build.

@denyskon
Copy link
Member

denyskon commented Sep 4, 2025

I think build dependencies still count as dependencies

And labels document the PRs, so I think them being part of "docs" is fine.

@techknowlogick
Copy link
Member Author

I think build dependencies still count as dependencies

And labels document the PRs, so I think them being part of "docs" is fine.

Yup, this was my thinking on it too

@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 Sep 5, 2025
@wxiaoguang
Copy link
Contributor

I think build dependencies still count as dependencies
And labels document the PRs, so I think them being part of "docs" is fine.

Yup, this was my thinking on it too

TBH I don't agree.

And I don't understand if you agree, then why not make "docs" label be automatically added when such file changes?

@silverwind
Copy link
Member

silverwind commented Sep 5, 2025

I will probably eliminate tools/package.json by replacing fabric with https://github.com/ssssota/svg2png-wasm. It being wasm means there are no native dependencies, so it will be safe to ship in the main package.json.

The fabric dependency is only used for generating these png images and users who want to build their own images for their instance are running this as well.

Edit: package above is deprecated on npm, so https://github.com/thx/resvg-js#webassembly appears to be the next best thing.

@silverwind
Copy link
Member

TBH some labels seem noise to me, for example: "api" (a lot of unrelated), "cli" (a lot of unrelated), "internal" (unclear what it is for), "go" (most PRs change go files), "frontend" (tmpl also has frontend layout)

-modifies/internal is unclear, yes. Would remove it
-modifies/frontend: I would split into modifies/js and modifies/css
-modifies/go: imho keep it
-modifies/api: might be a good indicator to ensure we check for breaking changes
-modifies/cli: no real opinion from me here

@wxiaoguang
Copy link
Contributor

It seems that many (IMO at least half of them) are not really related to the labels (shouldn't be reviewed from the label's perspective). And even without "api" label (no indicator), we should always pay attention to the breakings for every PR.

@silverwind
Copy link
Member

Big refactors will always have all labels, it's unavoidable. I guess I agree to removing cli as well if its not helpful.

@techknowlogick
Copy link
Member Author

Closing this due to the wasm PR that would make it unnecessary

auto-merge was automatically disabled September 5, 2025 20:08

Pull request was closed

@techknowlogick techknowlogick deleted the techknowlogick-patch-9 branch September 5, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/internal skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants