-
Notifications
You must be signed in to change notification settings - Fork 27.9k
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
Spelling #47894
Spelling #47894
Conversation
@@ -15,7 +15,7 @@ import { FoldingRange } from 'vscode-languageserver-protocol-foldingprovider'; | |||
import { getLanguageModelCache, LanguageModelCache } from '../languageModelCache'; | |||
import { getDocumentRegions, HTMLDocumentRegions } from './embeddedSupport'; | |||
import { getCSSMode } from './cssMode'; | |||
import { getJavascriptMode } from './javascriptMode'; | |||
import { getJavaScriptMode } from './javascriptMode'; |
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.
In general, JavaScript
is a brand and there are only two proper ways to spell it: JavaScript
, or javascript
. As javascriptMode
is probably a module, I probably should drop this.
@@ -76,7 +76,7 @@ export function testCompletionFor(value: string, expected: { count?: number, ite | |||
} | |||
|
|||
suite('HTML Completion', () => { | |||
test('HTML Javascript Completions', function (): any { | |||
test('HTML JavaScript Completions', function (): any { |
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.
Here's a case where JavaScript
should be properly spelled...
@@ -67,7 +67,7 @@ async function showPreview( | |||
|
|||
telemetryReporter.sendTelemetryEvent('openPreview', { | |||
where: previewSettings.sideBySide ? 'sideBySide' : 'inPlace', | |||
how: (uri instanceof vscode.Uri) ? 'action' : 'pallete' | |||
how: (uri instanceof vscode.Uri) ? 'action' : 'palette' |
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 know that a lot of the changes here are to telemetry calls, which presumably will impact a database somewhere.
I'd argue that changing to the correct spelling and at worst adding a shim/collator for the backend is the right thing to do.
That said, I'm happy to drop these things if requested.
@@ -5163,7 +5163,7 @@ declare module "crypto" { | |||
sign(private_key: string | { key: string; passphrase: string }): Buffer; | |||
sign(private_key: string | { key: string; passphrase: string }, output_format: HexBase64Latin1Encoding): string; | |||
} | |||
export function createVerify(algorith: string): Verify; | |||
export function createVerify(algorithm: string): Verify; |
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'm assuming that changes like this aren't a significant thing.
@@ -400,7 +400,7 @@ class TokenTreeBuilder { | |||
|
|||
/** | |||
* Parses this grammar: | |||
* grammer = { line } | |||
* grammar = { line } |
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 hope this isn't a problem :-)
@@ -123,7 +123,7 @@ export class OneSnippet { | |||
} | |||
} | |||
|
|||
// change stickness to never grow when typing at its edges | |||
// change stickiness to never grow when typing at its edges |
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'm not actually sure about this one
@@ -155,7 +155,7 @@ suite('Instantiation Service', () => { | |||
assert.ok(collection.has(IService2)); | |||
}); | |||
|
|||
test('@Param - simple clase', function () { | |||
test('@Param - simple case', function () { |
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'm not sure about this
@@ -171,12 +171,12 @@ async function ensureVersionAndSymbols(options: IOptions) { | |||
} | |||
|
|||
// Environment | |||
const pakage = require('../../../package.json'); | |||
const pkg = require('../../../package.json'); |
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 understand that package
is a reserved word.
Generally, instead of a random misspelling, people use an abbreviation, I've picked pkg
(and Travis doesn't seem to object). I'm flexible, in case you have a preference. Other alternatives include using a _
...
@jsoref We don't take PR's that don't bring any improvement to the product itself. Typos are OK to have, especially in comments. If you want to contribute to the project I suggest to go through https://github.com/Microsoft/vscode/wiki/How-to-Contribute |
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.
@joaomoreno: thanks for your feedback.
I'm not going to argue what does improve a given product, that's of course your project's choice. My personal view is that misspellings make it harder to search for things and harder to understand, contextualize, localize, or even fix things.
What would be helpful is a sense of which things do "improve" your product per your definition.
As a potential English speaking user of your project, I'd expect that user interface elements and messages would be improved by having correct spelling, would changes to that effect count as an improvement per your definition?
} | ||
return null; | ||
}, null, `Error while computing signature help for ${signatureHelpParms.textDocument.uri}`, token); | ||
}, null, `Error while computing signature help for ${signatureHelpParams.textDocument.uri}`, token); |
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.
This feels like output, would you take changes like this?
@@ -972,8 +972,8 @@ export class CommandCenter { | |||
|
|||
if (unsavedTextDocuments.length > 0) { | |||
const message = unsavedTextDocuments.length === 1 | |||
? localize('unsaved files single', "The following file is unsaved: {0}.\n\nWould you like to save it before comitting?", path.basename(unsavedTextDocuments[0].uri.fsPath)) | |||
: localize('unsaved files', "There are {0} unsaved files.\n\nWould you like to save them before comitting?", unsavedTextDocuments.length); | |||
? localize('unsaved files single', "The following file is unsaved: {0}.\n\nWould you like to save it before committing?", path.basename(unsavedTextDocuments[0].uri.fsPath)) |
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.
This is definitely a user visible message. Would you take a series that only changed the English output for nls.localize(..., "...")?
@jsoref Absolutely! You can submit PRs fixing up user-facing labels, we'll take them gladly! |
Hi. I don't expect you to want to take this PR as is.
I'm quite happy to split this into a number of different PRs, to collapse commits, and to perform other tasks as requested.
If there are certain files that you don't want changed, I can certainly drop them.
For reference, I ignore certain files -- by deleting them -- you can see which I ignored at: https://github.com/jsoref/vscode/tree/ignore
In general, I try to keep commits unsquashed until a PR is ready to be accepted, because it makes it easier to manage (rebase) merge conflicts or to drop a word in case it turns out it's intentional.
Often, I'm asked to split off PRs by categories, such as:
If you have a preferred set, please let me know.