Enforce the Attribute suffix in AttributeName

[fourls]

The AttributeName rule currently allows you to configure whether the "Attribute" suffix is required, optional, or forbidden. This is unnecessarily flexible as there is no good reason not to use the suffix - and makes it difficult to write a rule description for as it can be all things to all people. Attributes should be named with the Attribute suffix because

• It is a clear naming convention for attribute classes, which should not be used in other ways
• It ensures that the attribute is never shadowed by another class
• The .NET docs recommend it

This PR removes this configuration from AttributeName, instead always requiring the "Attribute" suffix.

[Cirras]

Looking good, can you update the CHANGELOG?
Just the rule improvement, no need to list the metadata changes in the second commit.

[fourls]

Are we updating the CHANGELOG in PRs? I haven't done that in any of my other PRs.

I understand how this could be useful, ensuring that the CHANGELOG is up to date, but it also has downsides - in particular, requiring PRs to update the CHANGELOG has the disadvantage of causing regular merge conflicts when accepting pull requests.

If we are going to enforce updating it in the PR, then I should also update the contribution guidelines in #49.

[Cirras]

No strict decision on that one yet, you're right that it would be an area rife with merge conflicts.
We could make it a strict policy not to update the changelog in PRs and make it the responsibility of whoever rebases and merges the PR into master.

Thoughts? @zaneduffield @fourls

[fourls]

I think it would be a good idea to forbid updating the changelog in PRs, just because of the potential friction to the PR workflow.

The only problems are

1. We would then be pushing directly to master quite a lot, and
2. If we forget to do so, then things could get missed.

We can edit the CHANGELOG through the GitHub UI, so hopefully (2) shouldn't cause too much friction for the merger.

Could it be possible to have some sort of automated process for updating the changelog? Off the top of my head, I'm not sure exactly what that would look like.

[Cirras]

The only problems are

1. We would then be pushing directly to master quite a lot, and
2. If we forget to do so, then things could get missed.

I was thinking that the PR reviewer would rebase and force-push the changelog update to the PR branch before merging.

We can edit the CHANGELOG through the GitHub UI, so hopefully (2) shouldn't cause too much friction for the merger.

Could it be possible to have some sort of automated process for updating the changelog? Off the top of my head, I'm not sure exactly what that would look like.

Automation on this sounds like a bit of a pain, I wouldn't bother.

[zaneduffield]

This is a pretty common problem in active projects.

Some solutions include

1. Organising unreleased changelog entries into a directory structure and merging on release (gitlab)
2. Setting the merge=union gitattribute for the changelog file and performing the rebases locally (github doesn't support this natively - https://github.com/orgs/community/discussions/9288)

[fourls]

I was thinking that the PR reviewer would rebase and force-push the changelog update to the PR branch before merging.

If the reviewer will have to manually rebase the changelog update anyway, you might as well just enforce updating the changelog in the PR.

1. Organising unreleased changelog entries into a directory structure and merging on release

This seems like a good solution for big projects, but probably overkill for SonarDelphi.

2. Setting the merge=union gitattribute for the changelog file and performing the rebases locally (github doesn't support this natively - https://github.com/orgs/community/discussions/9288)

This would be perfect! Annoying that GitHub doesn't support this natively, looks like it doesn't read .gitattributes at all.
We might as well add this to our .gitattributes anyway - if we enforce updating the changelog in the PR, it'll cause less merge conflicts if users rebase their PR branch onto master.

[fourls]

I was thinking that the PR reviewer would rebase and force-push the changelog update to the PR branch before merging.

We also won't necessarily have permissions to change the PR branch.

See GitHub docs:

You can only make commits on pull request branches that:

• are opened in a repository that you have push access to and that were created from a fork of that repository
• are on a user-owned fork
• have permission granted from the pull request creator
• don't have branch restrictions that will prevent you from committing

I think the best solution overall would probably be to have merge=union on .gitattributes and enforce a changelog update for PRs.

Personally, I think it would also work well enough (and be less messy) if we didn't enforce anything on PR and just periodically updated the changelog, but I can understand why we might not want to do that.

[zaneduffield]

I think at the very least the future changelog entries should be written in advance. I don't really care whether it is stored in the commit message, in a PR comment, or inside the repository.

[fourls]

The more I think about it, the more I think this will be the easiest workflow:

1. Contributor makes a PR and does not touch the changelog.
2. Reviewer merges PR into master.
3. On master, reviewer updates changelog using the GitHub UI.

The main reasons I have for this are:

• Our changelog is a product changelog, so we want to curate what it says. We don't want contributors writing their own interpretations of what is useful or significant about the latest release. We also don't want to lay that responsibility on them - I can definitely see instances where review will go back and forth purely on the changelog description.
• We should also not (and possibly will not be able to) change another user's PR branch. Any changes we do should be in master after the PR is merged.
• This workflow is simple and does not require undue effort on the contributor's part.
• There are easy ways to add automations to improve this experience for the reviewer - for example, a bot that comments on just-merged PRs with the URL to edit the changelog.

[Cirras]

The main reasons I have for this are:

• Our changelog is a product changelog, so we want to curate what it says. We don't want contributors writing their own interpretations of what is useful or significant about the latest release. We also don't want to lay that responsibility on them - I can definitely see instances where review will go back and forth purely on the changelog description.
• We should also not (and possibly will not be able to) change another user's PR branch. Any changes we do should be in master after the PR is merged.
• This workflow is simple and does not require undue effort on the contributor's part.
• There are easy ways to add automations to improve this experience for the reviewer - for example, a bot that comments on just-merged PRs with the URL to edit the changelog.

Yep, this reasoning is pretty compelling to me.
I don't mind if we directly modify a PR branch right before merging it, and I also don't mind if we have a separate commit following it to update the changelog.
Either way is fine, as long as it's the reviewer's responsibility.

[fourls]

Sounds good! Is this PR okay to be merged?