Skip to content

Update general guideline #746

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

Merged
merged 2 commits into from
May 26, 2025
Merged

Update general guideline #746

merged 2 commits into from
May 26, 2025

Conversation

slickepinne
Copy link
Member

We updated the general guidelines and moved language specific guidelines to the relevant folders.

Co-authored-by: Valeria Graffeo [email protected]
Co-authored-by: Rob Whittaker [email protected]
Co-authored-by: Trésor Bireke [email protected]

Co-authored-by: Valeria Graffeo <[email protected]>
Co-authored-by: Rob Whittaker <[email protected]>
Co-authored-by: Trésor Bireke <[email protected]>
@thoughtbot-github thoughtbot-github force-pushed the update_general_guideline branch from eb96218 to 60e3c61 Compare April 4, 2025 13:44
Copy link
Contributor

@JoelQ JoelQ left a comment

Choose a reason for hiding this comment

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

This feels like a mix of multiple unrelated changes:

  1. Delete Hound guideline
  2. Move some guidelines out general
  3. Reformat part of general
  4. Change guideline on whitespace

Thoughts on making each of these their own commit? Maybe even their own PR? That would allow conversation to happen independently for each, and if one proves controversial then it doesn't block merging the others.

Comment on lines -26 to +23
- Delete trailing whitespace.
- Delete trailing spaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace is more than just spaces. It includes other invisible characters such as tabs, newlines, carriage returns or the unicode zero-width-joiner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would "blank spaces" include these characters? Our main goal was to not use "whitespace".

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but I think it's more ambiguous because some of the characters aren't spaces.

"Whitespace" seems to be the precise technical term used in e.g. the whatwg spec, the ECMAScript spec, or the Unicode spec to describe the class of characters (although they all define the set slightly differently).

Is "whitespace" in the context of characters and typography considered problematic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also want to ensure we're covering more than just trailing spaces as there's many other characters that one should avoid trailing a line.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JoelQ Not sure about being considered problematic but in our playbook and best practices chapter, we write:

Use negative space instead of white space.
Of course, it's important to consider the "environment" of where we use a terminology – I just feel if there is a more inclusive naming that captures the same intent, I would prefer using it.

@vburzynski How do you feel about adding these other characters we should avoid to this list?

Copy link
Contributor

Choose a reason for hiding this comment

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

TL;DR: Perhaps "delete trailing spaces" is the better phrasing if we do need to get rid of "whitespace".

Whitespace is the more precise technical term, but there are instances where "trailing whitespace" is shortened to "trailing spaces". I would, however, strongly argue against using "blank space" or "negative space" over "whitespace".

In general, I agree with attempting to use more inclusive verbiage where possible. Yet, in regards to this use case, I think the alternatives muddle the clarity of the guideline. There are instances, however, where trailing whitespace is referred to as trailing spaces so that option would be acceptable (see the link to ESLint below for an example).

More often than not, the trailing characters are spaces, but they could also be many other characters. A whitespace character (no space), as Joel mentioned, is a specific technical concept. There are 25 Unicode characters that fall under the category (see https://en.wikipedia.org/wiki/Whitespace_character)

Whitespace refers to characters which are used to provide horizontal or vertical space between other characters. Whitespace is often used to separate tokens in HTML, CSS, JavaScript, and other computer language
https://developer.mozilla.org/en-US/docs/Glossary/Whitespace

On the other hand, White Space (the visual arts concept) is more closely related to the concept of Negative Space. There are still slight nuances between the two concepts, but are often interchangeable in the context of design and art. Negative space is the unused space in relation to one or more subjects within that space. White space is the unused or unmarked space. So they're talking about the same thing, but using two different frames of reference.

  • trailing whitespace characters would be the most precise phrase
  • trailing spaces would be less precise, but acceptable and has demonstrable usage
  • negative space is not used to describe whitespace characters in the context of programming.
  • blank space feels too ambiguous
  • white space is either a different visual arts concept or a misspelling of whitespace.

example usage of "trailing spaces"

example usage of "trailing whitespace"

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think whitespace was to be removed as per inclusive language, but if that's the case, I'm in favor of "trailing spaces" as well, FWIW.

Copy link
Contributor

@purinkle purinkle left a comment

Choose a reason for hiding this comment

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

🤵 Looks good to me!

@heyvaleria
Copy link
Contributor

@slickepinne I think we can merge this 😃

@heyvaleria heyvaleria merged commit 083e2e6 into main May 26, 2025
2 checks passed
@heyvaleria heyvaleria deleted the update_general_guideline branch May 26, 2025 07:53
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.

6 participants