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.

Sign up for GitHub

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
+33 −31
Open

Cleanup some issues with the coding standards #554

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 33 additions & 31 deletions coding_style_and_standards.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,20 @@ maintained by the ManageIQ team.

* We follow, with some exceptions, the [original Ruby style guide](https://github.com/bbatsov/ruby-style-guide).
Any changes we have that deviate from the default style guide are enumerated
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 •
edited
Loading

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)


## Documentation

We use [yardoc](https://yardoc.org/) to create inline code documentation. For now documentation is scarce but
we encourage contributors to add it whenever possible. Until more of the codebase is documented, we limit the created
documentation to those files that are well documented.
we encourage contributors to add it whenever possible. Until more of the codebase is documented, we limit the created
documentation to those files that are well documented.
Please try to document methods that are used by external teams, like providers.
These are the integration API for them and should be documented best.

To view the documentation online visit [rubydoc.info](http://www.rubydoc.info/github/manageiq/manageiq). For a local
version you can run `bundle exec yard` and view the html docs in `/doc/index.html`. When writing documentation you
should add your source files to `/.yardopts` and run `bundle exec yard server -r`. This will create a local
should add your source files to `/.yardopts` and run `bundle exec yard server -r`. This will create a local
documentation server which regenerates the docs on each request.

## Logging
Expand Down Expand Up @@ -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 •
edited
Loading

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.

* A short summary of the commit under 72 characters. Do not use a ticket
number as the subject alone.
* A blank line.
Expand All @@ -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

there are multiple.
* Bugzilla tickets should be in the form of a full URL to the ticket.
* Github issues should be of the form "Issue #n", where n is the
issue number.
* GitHub issues should be of the form "Issue #n", where n is the issue number.
* Other tickets should be in the form of a full URL to the ticket or the form
appropriate for that system.
* Each commit should have its own unique subject. Do not use the same
subject for a series of commits in a branch of work.
* Avoid using [@-mentions](https://github.com/blog/821) in the body of the commit
message as GitHub will notify the user every time the commit is pushed. As a
corollary, code that uses `@` could accidentally be interpreted as an @-mention,
so all code snippets in commit messages should be marked within code block
formatting.
* Keep commits small by committing often and only include related changes
and tests together.
* You may be doing too much in a commit if...
* You can't get the subject of the commit message under 72 characters
* You are using a lot of "ands" or "ors" in the commit message
* You are using a lot of "and"s or "or"s in the commit message
* You find yourself using lots of bullet points to enumerate all the work
the commit does.
* Squash (combine) commits to keep logical units of changes grouped together
Expand All @@ -138,7 +142,7 @@ documentation server which regenerates the docs on each request.
change_ems_amazon_to_be_region_specific
```

* Write a good pull request message. By default, Github will use your
* Write a good pull request message. By default, GitHub will use your
branch name as the title. Adjust the title if this is not appropriate for
the pull request.
* See [writing a good commit message](#commits) for information about
Expand All @@ -153,7 +157,7 @@ documentation server which regenerates the docs on each request.
code smells.
* Try to avoid having a commit with, for example, a spelling mistake, that
is fixed in a subsequent commit in the same pull request. Use `git rebase -i`
to clean up those commits to keep the history clean.
to squash those commits to keep the history clean.
* If a pull request involves UI changes, consider adding a before/after
set of screenshots to show what has changed visually.
* Use [@-mentions](https://github.com/blog/821) to request reviews from
Expand All @@ -164,7 +168,7 @@ documentation server which regenerates the docs on each request.
## Error and Issue Reporting

* Under no circumstances should customer names or customer related information
be referenced in Github issues, error reports, commits, or pull requests.
be referenced in GitHub issues, error reports, commits, or pull requests.
* For UI errors, the error message and stack trace are usually in
production.log. A snippet from there with the entire UI transaction is
needed, including the error message and the stack trace. A UI transaction
Expand Down Expand Up @@ -193,7 +197,7 @@ documentation server which regenerates the docs on each request.

Most items are handled by a queue worker of some type, so for those it
is most useful to have:

* The MiqQueue.put or MiqQueue.merge line from the worker that placed
the item on the queue.
* Context around why the item was placed on the queue. For example, if
Expand Down Expand Up @@ -229,44 +233,44 @@ When extracting code into a new gem or creating a new gem:

## Git how-to

Note after the changes in this section, you will need `git push -f `if you have
Note after the changes in this section, you will need `git push -f `if you have
already pushed them before.

* Reword/squashing/reordering a commit

To modify with recent commit in current branch, first do
To modify with recent commit in current branch, first do
`git rebase -i origin branch-name`.
To modify a specific commit, use `git rebase -i SOME_COMMIT_ID^` instead.
git will popup a vi window to let you do modification on commits, press
git will popup a vi window to let you do modification on commits, press
`:wq` after done.

* Reword a commit
Change the `pick` before the commit you want to reword to `edit` and edit

Change the `pick` before the commit you want to reword to `edit` and edit
its message in popup vi window.

* Squashing commits
Change the `pick` before the commit you want to squach to `squash` and edit
the commit message after squash in a following popup vi window. A commit

Change the `pick` before the commit you want to squach to `squash` and edit
the commit message after squash in a following popup vi window. A commit
will be squashed with its previous commit.

* Reordering commits

Reordering them in this vi window will reorder the commits.

* Amend a commit

You can commit first and rebase it in the previous section. Or if you want to
amend most recent commit, you can: `git commit some_file --amend`.

* Deleting a commit

You can delete commits by delete corresponding lines in `git rebase`. Or if
you want to delete most recent commit, you can `git reset --hard HEAD^`. If you
want to go back to a specific commit and delete commits after that, use
`git reset --hard commit-hash`.

* Uncommit a file from an existing commit

```
Expand All @@ -277,8 +281,6 @@ already pushed them before.
## TODO

* Rails style guide see https://github.com/bbatsov/rails-style-guide
* Bugzilla how-to
* how to copy commit details / clone a ticket

# License

Expand Down