-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Make the strings use gooder English #5281
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
base: master
Are you sure you want to change the base?
Conversation
|
Some notes:
I started off making a list of line numbers with issues, until I realized how much of a massive waste of time it was due to how many instances like this there are, and time would be better spent just directly fixing them. |
Maybe it's not more grammatically correct, but it looks prettier if it's consistent and we can keep iterating on the grammar :D
Fixed
I'm not educated enough in proper grammar to weigh in on this if it's going to need a larger change across many strings. I'd need either references or someone else to confirm.
For the "why": Because these are written over time by different people and that makes it easy for inconsistencies to develop. It's always nice to have someone look at it all together to point these things out :) Making the tense consistent is not as simple/quick as a search-and-replace, so I probably won't include it in this PR |
Basically if a sentence is dropping the subject, the only case where it's technically appropriate to do so (in English) is in what's called the "imperative mood", which generally takes the form of a command, for example, sentences like "Run!" or "Sit down.". These are always in second person, and the "You" is implied, e.g. "You sit down.". By using the 3rd person singular form of a verb, you're establishing that it's not in the imperative mood, e.g. "Allows a reboot if applicable.". This isn't seen as valid sentence structuring in English as the subject being dropped is 3rd-person, not 2nd. The full sentence being implied here is "This allows a reboot if applicable.", and that's not a case where it's acceptable to drop the subject in English. However, this is done quite frequently in "documentation" type settings, which is why I stated in the other PR that it's basically a philosophical debate at this point, as it's widely done but technically incorrect. Either way, it should go one way or the other.
Oh, I know. My "why" was very rhetorical haha. I'd need some time, but I could definitely go through that whole document and make a definitive "Pedant's Edit" at some point in the near future lol |
|
@pressRtowin I didn't see the other PR you made. THANK YOU SO MUCH! There are so many cases where we've failed to be consistent. It's been a challenge to prioritize cleaning up the mess. I really appreciate the time taken to review/improve the situation. I was just talking with another PM this afternoon about the UX for the WinGet CLI. I'm going to point her at this PR as well just for the sake of keeping her informed. I don't want to introduce any breaking changes, but the other area I've struggled with is in the naming of the commands. Most of them are verbs like "install" and "upgrade". Some of them are nouns like "settings" and "features". A couple of them could be ambiguous like "hash" and "pin". |
|
I'd also like to chime in, especially with regards to this specific thought -
In some cases, and the example here is a great one, the intent is truly that the object of the sentence is the plural |
Glad I could help! And yeah, consistency with parts of speech like that can really help especially if you're not familiar with the tool and trying to guess/remember commands without referring to the docs over and over.
I had assumed "Override" was a verb here, as many of the other descriptors start with a verb, but yes, since "Override arguments" is functioning as a noun phrase here, it's fine (though maybe rewording it for clarity might be warranted?). There are other examples of the issue I was describing though, such as "Unable to create (the/a) symlink. (The) Path points to a directory.", "Specify (the/a) authentication window preference (silent, silentPreferred, or interactive)", and "Failed to add (the/a) source. This winget version does not support the source's authentication method. Try upgrading to latest (the) winget version.", etc. |
Thank you for mentioning |
|
Talked with @JohnMcPMS offline. For argument descriptions the main priority is to keep them short since the terminal has a limited space, even if it is not perfectly grammatical. I'm leaving argument descriptions with no periods as that's what most already had. For consistency, I removed periods from the few arguments that had it, and changed the few ones that were two "sentences" to be only one. For the errors it makes sense to me to have them be more proper English, plus it aligns with other Windows error codes. |
| </data> | ||
| <data name="NoApplicableInstallers" xml:space="preserve"> | ||
| <value>No applicable installer found; see logs for more details.</value> | ||
| <value>No applicable installer found. See logs for more details.</value> |
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.
Shouldn't this be "No applicable installer was found."?
| </data> | ||
| <data name="PortableRegistryCollisionOverridden" xml:space="preserve"> | ||
| <value>A portable package with the same name but from a different source already exists; proceeding due to --force</value> | ||
| <value>A portable package with the same name but from a different source already exists. Proceeding due to --force.</value> |
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.
Technically, "Proceeding due to --force." is a fragment (no subject), but I'm not sure what the best way to fix it would be. Something like "The process will continue due to --force." maybe?
| </data> | ||
| <data name="SourceOpenPredefinedFailedSuggestion" xml:space="preserve"> | ||
| <value>Failed to open the predefined source; please report to winget maintainers.</value> | ||
| <value>Failed to open the predefined source. Please report to winget maintainers.</value> |
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.
Same as for line 601.
Additionally, when "report" is used as an intransitive verb, it would mean "to check in" as in "report to the office". So unless we want people just checking in with the winget team, we probably want the transitive "report" (to submit an account of something), so an object is required (with another minor tweak as well): "Please report this to winget's maintainers."
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.
I changed it to say "winget's maintainers" but the style guide says
Note Don't use the possessive form of Microsoft trademarks and product, service, or feature names.
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.
Note Don't use the possessive form of Microsoft trademarks and product, service, or feature names.
"Please report this to those who maintain winget."? 😅
" . . . to the maintainers of winget." might work. Adheres to the guide without sounding biblical.
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.
I just laughed out loud and almost ended up wearing my coffee.
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.
I suppose this should be "WinGet" then rather than "winget" as it's referring to the maintainers of the product, not the command itself.
| </data> | ||
| <data name="PortableInstallFailed" xml:space="preserve"> | ||
| <value>Portable install failed; Cleaning up...</value> | ||
| <value>Portable install failed. Cleaning up...</value> |
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.
"The portable installation failed." probably? "Cleaning up." is also technically a problem but I have no clue what to replace that with.
| </data> | ||
| <data name="PortableHashMismatchOverridden" xml:space="preserve"> | ||
| <value>Portable package has been modified; proceeding due to --force</value> | ||
| <value>Portable package has been modified. Proceeding due to --force.</value> |
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.
"The portable package . . . "
Also, as with line 1394, "The process will continue due to --force." may be preferable.
| <value>No package selection argument was provided. See the help for details about finding a package.</value> | ||
| </data> | ||
| <data name="PortableAliasAdded" xml:space="preserve"> | ||
| <value>Command line alias added:</value> |
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.
Wasn't edited here, but does this fall under the scope? If so, "A command line alias was added" probably?
| </data> | ||
| <data name="PortableHashMismatchOverrideRequired" xml:space="preserve"> | ||
| <value>Unable to remove Portable package as it has been modified; to override this check use --force</value> | ||
| <value>Unable to remove Portable package as it has been modified. To override this check use --force.</value> |
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.
"The portable package cannot be removed as it has been modified. To override this check*,* use --force."
| </data> | ||
| <data name="InvalidPathToNestedInstaller" xml:space="preserve"> | ||
| <value>Invalid relative file path to the nested installer; path points to a location outside of the install directory</value> | ||
| <value>Invalid relative file path to the nested installer. Path points to a location outside of the install directory.</value> |
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.
"The relative file path to the nested installer is invalid. The path points to . . . "
|
It's been a while for me (I remember the Microsoft Manual of Style), but it looks like the latest "writing" guidance for US English is https://learn.microsoft.com/en-us/style-guide/welcome/ |
I checked out this page: https://learn.microsoft.com/en-us/style-guide/grammar/verbs Unfortunately it's not as helpful as I had been hoping it'd be for determining when to use the indicative vs the imperative. I don't know if the most recent proposed changes addressed any of this, but there was a lot of mix between the two, especially when describing options and parameters, as I mentioned before. For what it's worth, PowerShell is also quite inconsistent with this, so looking to them for guidance doesn't exactly help either. ( Then again, they do use periods for all those, and it seems we just elected not to, so I'm not sure how much consistency between Microsoft products we're aiming for. |
|
AHA! Found it! https://learn.microsoft.com/en-us/style-guide/developer-content/formatting-developer-text-elements Under "UI text or strings", the provided examples are all in the imperative:
They also don't use periods here (despite them being proper sentences due to the imperative mood), but they don't actually explicitly say to use or not use periods anywhere on that chart, despite examples given in other categories (such as for error messages) all consistently either using or not using periods for the whole category. |
|
Marking this as a draft because clearly there are still a lot of strings to fix 😅 |
|
My 2 cents: As long as we're consistent with "ourselves" in WinGet I think we're heading in the right direction. I'd like to be a great example for others to follow (assuming we're doing it the right way and honoring the guidance given). Given the problems we have with terminal width and how word wrapping is implemented, the main reason to omit trailing periods is to avoid an unwanted line feed. |
But I love reading text like But yeah that makes sense, and seems to be in line with what the guide (silently) suggests. On that note though, is it "winget" or "WinGet"? |
| </data> | ||
| <data name="InvalidPathToNestedInstaller" xml:space="preserve"> | ||
| <value>Invalid relative file path to the nested installer; path points to a location outside of the install directory</value> | ||
| <value>Invalid relative file path to the nested installer. The path points to a location outside of the install directory.</value> |
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.
"The relative file path to the nested installer is invalid."
|
"WinGet" is the product name. There are some scenarios where the lower case makes sense in terms of one referencing the "winget" command. It depends on whether one is referring to the product or the executable. It's also valid to have a reference to the WinGet CLI (Command Line Interface). <- Yes I have a hard time using WinGet Command Line Interface (CLI) properly too. |



Off-shot of @pressRtowin's #5279
This adds periods at the end of all string resources for argument descriptions and error codes. It also replaces a bunch of semicolons by periods.
Microsoft Reviewers: Open in CodeFlow