-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fix typo in dotnet-tools/README.md #19021
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
Conversation
For an eventual bulk typo fix PR. Can I have a clarification of what the fix is here?
Can I have a clarification of what the fix is here?
Concomitant with typo fixes in PowerShell/PowerShell would removal of a word from .spelling be authorised?
The merged PR above had a link to a page where
The spelling in the commit message was of course incorrect and that spelling found its way into .spelling.
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If over 200 lines changed should that be in multiple PRs? |
@spaette Thanks for your contribution!
The best practices are not to mix functional and style changes. This makes code review easier and saves time - this is the main criterion to follow when determining the optimal size of changes and their grouping by PRs and by commits. For example, if you fix one and the same typo in 1000 files, you can do it in one PR but do 20-30 commits with 20-50 files. |
Could you respond to the questions I addressed to @PaulHigin? With those questions answered I could add relevant lines to the below shell script. Then accordingly the script could be a basis for a single PR.
|
It is script. We shouldn't change it.
I guess
We should also change the commit description. I suggest you pull separate PR to change the files (7.1.md and .spelling) and ask MSFT team to change the commit too. |
I'm unclear on your meaning. Is that a single PR with multiple commits or multiple PRs? A recursive grep for
Your position is noted. Other *.ps1 files appear in typos.sh.
That was my inclination.
I've seen elsewhere a reluctance expressed to change a commit message following a merge. a few more typos
sed -i "s/Custome/Custom/g" PowerShell/src/PowerShell.Core.Instrumentation/PowerShell.Core.Instrumentation.man
sed -i "s/Custome/Custom/g" PowerShell/src/System.Management.Automation/resources/EventResource.resx
sed -i "s/custome/custom/g" PowerShell/test/powershell/engine/Module/ModulePath.Tests.ps1
sed -i "s/catched/caught/g" PowerShell/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs |
Can that be more precise. Which extension(s) on files |
There is no definite answer. Most of the files where we would see typos are special kind of program files. Even the comments there are not critical. But there are kinds of comments that translate into documentation, such as triple slash XML C# comments, where typos should be corrected. There are also resource strings in resx files, which are XML files. This would also be worth correcting as it is exposed to users. |
A typo in an .resx file also appears in a .man file. I don't know whether the .man file is generated. The code under It's unclear in this special case whether both files where the typo appears should be fixed in this repo. src/System.Management.Automation/resources/EventResource.resx
src/PowerShell.Core.Instrumentation/PowerShell.Core.Instrumentation.man
|
Resx and man files can contain specific names like |
🎉 Handy links: |
test/perf/dotnet-tools/README.md