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

chore: minor fixes to install-misspell.sh and typo in readme #26

Merged

Conversation

jessp01
Copy link

@jessp01 jessp01 commented Jan 26, 2025

README: seprated->separated
install-misspell.sh:

  • Make use of PROJECT_NAME
  • -n instead of ! -z
  • jq can accept filename as an argument so no need to pipe from cat

install-misspell.sh:
- Make use of `PROJECT_NAME`
- `-n` instead of `! -z`
- `jq` can accept filename as an argument so no need to pipe from `cat`
@ldez ldez changed the title Minor fixes to install-misspell.sh and typo in README chore: minor fixes to install-misspell.sh and typo in readme Jan 26, 2025
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

I recommend avoiding the usage of organization forks to open a PR: the PRs from organizations cannot be modified by maintainers or automation.

I also recommend making one PR by topic (typos, script).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ldez ldez added the enhancement New feature or request label Jan 26, 2025
@jessp01
Copy link
Author

jessp01 commented Jan 26, 2025

Hi @ldez ,

I recommend avoiding the usage of organization forks to open a PR: the PRs from organizations cannot be modified by maintainers or automation.

I happen to own this particular organisation but regardless, can you please elaborate as to what you mean by "cannot be modified by maintainers or automation"? You either have the needed privileges on a repo (and a pull) or you don't, unless I'm missing something, I'm not sure what actions cannot be done on a repo fork that is hosted under an org.

I also recommend making one PR by topic (typos, script).

Generally speaking, I utterly concur. However, as one change is very minor and only pertains to the README (and can therefore not break anything), I don't think it matters much in this particular case. Still, if you insist, I'm happy to do it:)

I've replied to your other comments in-line.

@ldez
Copy link
Member

ldez commented Jan 26, 2025

I happen to own this particular organisation but regardless, can you please elaborate as to what you mean by "cannot be modified by maintainers or automation"? You either have the needed privileges on a repo (and a pull) or you don't, unless I'm missing something, I'm not sure what actions cannot be done on a repo fork that is hosted under an org.

A maintainer or bot/automation cannot rebase or add a commit to a PR from an org fork.

FYI, I'm the owner of the golangci organization, so I have all rights on this repo, but even with the higher level of rights, the rights on PR from an org are limited (no push permissions).

However, as one change is very minor and only pertains to the README

I will not be picky here, so I will not ask you to split the PR, but for example: I can agree with typos and not script changes.
Minor changes are rare, and it's better to let the maintainers evaluate the importance of changes.

@jessp01
Copy link
Author

jessp01 commented Jan 26, 2025

Hi @ldez ,

A maintainer or bot/automation cannot rebase or add a commit to a PR from an org fork.

Understood. I typically fork onto my own user space anyway but in this case, it wasn't possible as I already forked the original repo (https://github.com/client9/misspell)

I will not be picky here, so I will not ask you to split the PR, but for example: I can agree with typos and not script changes. Minor changes are rare, and it's better to let the maintainers evaluate the importance of changes.

Again, agreed - in principle:)

I've pushed the change to line 247 of the README and replied to your last comment with respect to line 310.

@ldez
Copy link
Member

ldez commented Jan 26, 2025

I typically fork onto my own user space anyway but in this case, it wasn't possible as I already forked the original repo

If you delete the fork from the original repo, you can fork with your account.

The original repo is unmaintained so you don't need to keep a fork of it.

README.md Outdated Show resolved Hide resolved
BASH->Bash

Co-authored-by: Ludovic Fernandez <[email protected]>
@ldez
Copy link
Member

ldez commented Jan 26, 2025

A concrete example of the problem related to a fork from org: I cannot rebase your PR, but without a rebase, the CI will fail.

@ldez
Copy link
Member

ldez commented Jan 26, 2025

Note: I should trigger manually the CI after each of your commit 😉

Screenshot 2025-01-27 at 00-42-02 chore minor fixes to install-misspell sh and typo in readme by jessp01 · Pull Request #26 · golangci_misspell

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit 9801fd6 into golangci:master Jan 26, 2025
10 checks passed
@jessp01
Copy link
Author

jessp01 commented Jan 26, 2025

A concrete example of the problem related to a fork from org: I cannot rebase your PR, but without a rebase, the CI will fail.

Yes, I was not aware of this difference in functionality, frankly. Like I wrote, I do not normally clone onto the org myself.

Note: I should trigger manually the CI after each of your commit 😉

All checks passed just now.

RE: BASH vs. Bash - IMHO, all acronyms should be noted in all caps but, as GNU themselves use "Bash", I've no choice but to concede. I've done a most cursory search to see if I could find the reason for GNU's decision regarding the casing but it yielded no explanation. I might give it a few more minutes at some point, I'm stubborn like that:)

@ldez
Copy link
Member

ldez commented Jan 26, 2025

Yes, I was not aware of this difference in functionality, frankly. Like I wrote, I do not normally clone onto the org myself.

It's just annoying, not a major problem, I understand that you didn't know the problem.

I added comments to illustrate the limitations as you didn't know those limitations, I thought that was a good context to show concrete problems.
It was just to share the knowledge.

IMHO, all acronyms should be noted in all caps

Are you sure? Do you want to use LINUX or SED, LS, VIM, etc.?

FYI, Go is not an acronym 😉

@jessp01
Copy link
Author

jessp01 commented Jan 27, 2025

@ldez

Are you sure? Do you want to use LINUX or SED, LS, VIM, etc.?

Linux is not an acronym. SED is (Stream Editor) but I almost never write it like that because I am typically referring to the actual binary (when writing), LS is short for "list" but is not technically an acronym and I do spell VIM as VIM, unless I am referring to the executable itself.

@ldez
Copy link
Member

ldez commented Jan 27, 2025

So, Bash is not an acronym too, or you should write it BASh.

Linux is an acronym, a recursive acronym: Linux Is Not UniX
vim is Vi IMproved, so VIm.

@jessp01
Copy link
Author

jessp01 commented Jan 27, 2025

We're clearly both pedantic and stubborn people; as it seems that we're enjoying this discussion, I don't mind having it (GH can afford the very negligible storage and processing costs; I just hope notifications about this conversation will be sent to additional individuals as some may find this undesirable).

So, Bash is not an acronym too, or you should write it BASh.

You've got a point there but -
From https://www.gnu.org/software/bash:

Bash is the GNU Project's shell—the Bourne Again SHell.

And, the definition for an acronym is:

an abbreviation formed from the initial letters of other words and pronounced as a word

Notice the letters (plural) bit. It's somewhat ambiguous but can be interpreted to mean that you may use multiple [initial] letters of the words the acronym consists of.

Linux is an acronym, a recursive acronym: Linux Is Not UniX

I firmly believe that's a late addition and I'm not sure it was ever officially supported by Torvalds. I can check (or you can refer me to a doc stating otherwise if you have it). See https://en.wikipedia.org/wiki/Linux#Naming

In all my writings, I use BASH and VIM (when referring to the projects, not the binaries, of course) and I intend to continue to do so. Having said that, your repo, your rules:)

@ccoVeille
Copy link

ccoVeille commented Jan 27, 2025

Naming things the right way is a very interesting and could be painful.

So interesting that I started working on a personal project about this

https://github.com/jargonLint/jargonLint

I hope you will find interest in it.

I will add the Bash and VIm examples to it.

Please note my comment is not about promoting my project, but to let you know I appreciated your discussion and I'm happy to see other people share my thoughts about the fact brands, technologies ... should/could be written the right way

Thanks

@jessp01
Copy link
Author

jessp01 commented Jan 27, 2025

Hi @ccoVeille ,

I'm glad you found the discussion interesting. From how you spelt these two in your comment, I gather you agree with @ldez on the BASH/Bash point and have a third opinion on VIM (VIm)?

Personally, I also don't mind people promoting their projects (assuming they are relevant to the discussion at hand), I think it's both natural and sensible.

@ccoVeille
Copy link

I'm glad you found the discussion interesting. From how you spelt these two in your comment, I gather you agree with @ldez on the BASH/Bash point and have a third opinion on VIM (VIm)?

I'm sorry it was simply a typo here, it's Vim (checking on their official site confirm it) https://www.vim.org/

Personally, I also don't mind people promoting their projects (assuming they are relevant to the discussion at hand), I think it's both natural and sensible.

Feel free to continue the discussion on my project. There is no need to add noise to the following PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants