Skip to content

Centralize comment constructor #404

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

Closed

Conversation

JerryQ17
Copy link
Contributor

@JerryQ17 JerryQ17 commented Aug 1, 2025

@Kobzol proposed to attach CommentTag directly to the Comment struct (comment), so we need to give every comment a unique tag.

To make this easier, this PR makes Comment::new private, so that you cannot construct arbitary comments anywhere throughout the codebase. There are also some improvements:

  • Make Comment::new accept impl Into<String>
  • Remove duplicate use std::dmt::Write
  • Use itertools::Itertools::join instead of .collect::<Vec<_>>().join

Then we can give every comment a tag with their constructor's name, either this or next PR.

@Kobzol
Copy link
Member

Kobzol commented Aug 1, 2025

Oh, I should have been more clear in my previous remark. What I meant what that originally I was imagining such a comment tag system, but after I realized that we only need it for a single comment right now, I considered it to be overkill and an unnecessary complication. If we wanted to do something like this in the future, then I don't think that we need to make Comment::new private, right? We could just add e.g. another constructor like Comment::with_tag, or something like that.

Using impl Into<String> in the constructor makes a few call sites nicer, so I'm fine with that. It will also increase compilation time (which is already atrocious for bors) slightly, but it will probably only ever monomorphize &str and String, and the function is small, so that shouldn't be so bad.

I'm less sure about actually centralizing stuff in the comment.rs file. I originally thought that it might be useful to see all the comments at one place, but now I actually think it was not such a great idea and maybe we should move in the opposite direction. Since I created the Comment struct, it is trivial to find all comments (just find usages of the struct). There's no need to actually move stuff within a single file, even if their logic and text is actually relevant in the module that sends the comment.

Comments with complex logic (such as help or info) should definitely stay within their respective command/handler modules, and not be moved to the comment module. And even the simpler comments should probably be moved as close to the place where they are used as possible. I guess that the only thing that makes it worth it to be in comment.rs are comments that are actually used from multiple modules, but even these could be moved to shared functions that send the comment.

So actually I would maybe like to see the opposite change than what this PR does, or just no change at all, I think that the current comment system is fine and is not worth large churn/changes. Sorry.. 😅 Maybe if you plan to work on large-ish refactorings on changes on bors, it would be better to first discuss them e.g. in https://rust-lang.zulipchat.com/#narrow/channel/496228-t-infra.2Fbors/topic/Changing.20the.20way.20we.20detect.20completed.20CI.20builds/with/529490067, just to avoid situations where we might not see the value of the change or want to see the code move in a differentdirection.

@JerryQ17
Copy link
Contributor Author

JerryQ17 commented Aug 2, 2025

We could just add e.g. another constructor like Comment::with_tag, or something like that.

Ah, I misunderstood your thought before. I thought that you want a tag: CommentTag, but you actually want a tag: Option<Tag>.

So actually I would maybe like to see the opposite change than what this PR does, or just no change at all.

I think it worth a comment in comment.rs to reflect your idea.

@JerryQ17 JerryQ17 closed this Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants