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

Fix Typos #27325

Open
wants to merge 3 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

classicrocker883
Copy link
Contributor

@classicrocker883 classicrocker883 commented Jul 31, 2024

Description

  • Simply add ) to docs\ConfigEmbedding.md
  • Replace " with ' in cron: in .github/workflows/close-stale.yml | .github/workflows/auto-label.yml, which is the correct sytax
-  - cron: "30 8 * * *"
+  - cron: '30 8 * * *'

Requirements

Benefits

Configurations

Related Issues

@classicrocker883 classicrocker883 changed the title missing ')' in docs\ConfigEmbedding.md Fix Typos Aug 13, 2024
@gudvinr
Copy link

gudvinr commented Aug 14, 2024

What is the purpose of your changes? What issue this PR solves?
Not only this one but also other PRs that just "fix" typos.

I don't really care if marlin maintainers are fine with that but it just looks weird.
Although from personal perspective, this is just unnecessary noise. While not developing marlin, I sometimes skim through PRs for some useful things to test.

But more importantly, when you contribute to project this big this also leads to set of issues:

  • increased load on maintainers
  • reduced git blame usefulness
  • CI/CD time waste

These kind of changes are never necessary unless:

  • they come together with significant changes (i.e. you modify code with piggybacking typo fixes into these changes IF those are relevant to changed code)
  • wording itself is an error (e.g. variable with name max contains minimal value or comment is misleading)

Otherwise you create changes that will waste other people's time with no practical benefit. Considering that it's not the first PR that comes from you, it looks like you do have some strategy though.

@gudvinr
Copy link

gudvinr commented Aug 14, 2024

Replace " with ' in cron: in .github/workflows/close-stale.yml | .github/workflows/auto-label.yml, which is the correct sytax

YAML is superset of JSON, and in JSON " will be the correct token to wrap character sequence.

Also, according to YAML specs double quotes are defined as a way to define string (scalar in terms of YAML types).
Github doesn't specifically say which spec they use for workflow files, but on their documentation page they refer to "Learn YAML in Y minutes" which uses double quotes too.

So it is unclear what you mean by "correct syntax" if " is not incorrect.

@classicrocker883
Copy link
Contributor Author

classicrocker883 commented Aug 16, 2024

So it is unclear what you mean by "correct syntax" if " is not incorrect.

meaning if you look up how this is supposed to be, the correct usage, not only that but looking at similar files use that same syntax.

talk about a waste of time, look at how much you wrote for the very little changes, and now wasting my time. wow
this solves the issue of it being a typo, nothing major. if it were anything else I would have said so.

@classicrocker883
Copy link
Contributor Author

first I'm told not to piggypack code with other PR's because it would be irrelevant. to keep all the code separate. then I'm told keep it short because its easier and faster to get pulled/merged.

now I'm told beneficial changes, no matter how small, aren't accepted unless it solves world hunger.

well it is what it is. so what then? search for more Typos in order for this PR to be acceptable?

or put it into an unrelated random one?

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.

2 participants