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

Replace Config field 5-tuple with a struct #6334

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Rawk
Copy link
Contributor

@Rawk Rawk commented Sep 19, 2024

Add struct ConfigOption<T> to replace the 5-element tuple used for Config fields, to make the code more readable.

The tuple element 3 is actually true if the option is stable, the opposite of what the comment said.

Also changes IgnoreList::doc_hint to return "[<string>, ...]" instead of "[<string>,..]". It is now the same syntax as MacroSelectors::doc_hint.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 20, 2024

@Rawk just a heads up, the team strongly discourages merge commits. No need to change that right now, but moving forward please rebase your changes in order to get your branches up to date.

@Rawk
Copy link
Contributor Author

Rawk commented Sep 20, 2024

Thanks for the heads up. I have now rebased my recent pull request on master.

The default action was to merge, so i thought that was what i was supposed to do. I prefer to rebase.

You probably already know this, but there seems to be no way of specifying default action on GitHub. Instead, the master branch could set "Require linear history" here, and/or disable "Allow merge commits" here.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 20, 2024

Awesome! Yeah, the team knows about those settings, and we'd definitely turn them on if we didn't need to allow merge commits for our subtree syncs with the rust-lang/rust repo.

Element 3 is actually the opposite of what the comment says.

The linked issue is completed, so i removed the comment.
To make it more readable.
@ytmimi
Copy link
Contributor

ytmimi commented Sep 20, 2024

@Rawk appreciate you trying to keep the PRs up to date, though rebasing the PRs every time there are new changes in master is a little unnecessary. Especially if you've got multiple PRs in flight at the same time since merging one will require you (or me, or another maintainer) to rebase the others anyway so we're just needlessly triggering CI.

Not an issue, but another thing I wanted to call out. Though if there are merge conflicts I totally understand rebasing to address those issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants