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

Cleanup some issues with the coding standards #554

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Nov 15, 2024

  • Fix bad links or remove them
  • Drop Bugzilla references and refer to external ticketing systems more generically
  • Add a case about @-mentions in commit messages

@jrafanie Please review.

@@ -88,7 +88,6 @@ documentation server which regenerates the docs on each request.
## Commits

* Write a good commit message. The format for a commit message is as follows.
Also [read more](http://goo.gl/w11us) on writing good commit messages.
Copy link
Member Author

Choose a reason for hiding this comment

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

This link is dead and I have no idea what is was to see if there's a replacement. I tried searching for a good link to replace it, but I can't really find a good one.

Copy link
Member

@jrafanie jrafanie Nov 15, 2024

Choose a reason for hiding this comment

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

I think we have the gist of the link details in the lines that follow anyway.

- Fix bad links or remove them
- Drop Bugzilla references and refer to external ticketing systems more
  generically
- Add a case about @-mentions in commit messages
in the [.rubocop_base.yml](.rubocop_base.yml) file, which is inherited by most
projects in the ManageIQ organization.
in the [manageiq-style base.yml](https://github.com/ManageIQ/manageiq-style/blob/master/styles/base.yml) file,
which is inherited by most projects in the ManageIQ organization.
Copy link
Member

@jrafanie jrafanie Nov 15, 2024

Choose a reason for hiding this comment

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

Can we add something like

Style guides should be treated as only guidance. If we disagree with a style suggestion in a particular situation, it's perfectly fine to explain why it doesn't make sense there. Ultimately, they are here for us. These rules have grown and changed over time so they aren't law.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a line above about if you can give a reason for violating the guide then violate it. I can expand on the existing text, but it's earlier in the document. Even so, the goal here was to fix bad links and bugs, so IMO should be a separate PR. (However, I just added the @-mentions for funsies specifically for you haha)

@@ -100,18 +99,23 @@ documentation server which regenerates the docs on each request.
SHA references to other commits, and even raw data to make the purpose
of the commit clearer (e.g. "Was broken by commit 0f3a459b").
* A blank line if you've written a body.
* References to any Bugzilla tickets or Github Issues, one per line if
* References to any GitHub Issues or other ticketing systems, one per line if
Copy link
Member

Choose a reason for hiding this comment

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

Is it worthwhile saying:

Note: ManageIQ repositories are public. If you reference a public github issue or pull request, it will be referenced back to your issue, pull request or commit. If you want to link to a github page but not have it link back to where you posted it, you can use the GitHub link but prefix it with www such as www.github.com instead of just github.com. It will still get redirected to the correct place but without linking back to the place where you mentioned it.

Words are hard. Feel free to clean that up.

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole www thing seems overly complex in this context. I wonder if we could have like a Tips and Tricks page and then link to that for that purpose.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, how about ignoring the workaround and make people aware they will be backlinked from any github issue/PR they reference:

Note: ManageIQ repositories are public. If you reference a public GitHub issue or pull request, it will be referenced back to your issue, pull request or commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's a good one...I can add that

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.

2 participants