Skip to content

Conversation

@jsoref
Copy link
Contributor

@jsoref jsoref commented Feb 4, 2016

I'm wondering if you're interested in a series like this.
https://github.com/jsoref/TypeScript/commits/spelling-full
has the individual changes.

I can split it into pieces if requested.

@msftclas
Copy link

msftclas commented Feb 4, 2016

Hi @jsoref, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@DanielRosenwasser
Copy link
Member

I'm sorry, this is kind of crazy. Having a spell checker run on all files in a repository is not an acceptable sort pull request on its own, and not just for our project. I encourage you to please read all READMEs, CONTRIBUTING.mds, and coding guidelines for repositories you are intent on contributing to.

There are certain directories and files that are not expected to be changed. Specifically, check out README.mds for certain directories where files were changed - directories to watch out for are tests/cases, lib, and docs. Anything found in docs should get an issue.

Code was actually broken here. Generated names shouldn't be touched at all since they're, well, generated and for a reason.

Furthermore, several changes were actually wrong (i.e. "expressing" changed to "expressiong"). Please make an effort to supervise spelling changes before sending them out for review.

I understand that you have good intent, but reviewing this sort of thing takes a long time for team members and is error prone. It also makes code attribution harder (i.e. git blame for specific lines). We'd potentially consider taking in a few spelling corrections a day.

@RyanCavanaugh
Copy link
Member

I would love to see this broken up into reasonable chunks, probably one per gigantic file we have, a few that group up smaller source files, and then a separate PR for the error messages that has three commits -- one for diagnosticMessages.json, one for all the downstream changes in source, and one for the error baselines. That should be pretty easily reviewable (I ❤️ copy editing so... yea)

Also, don't bother changing anything under tests/cases. These are only ever seen by us and just excluding will help a lot.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 4, 2016

@RyanCavanaugh: if you could identify lists of files you'd like as sets, I can certainly do that.
as noted, https://github.com/jsoref/TypeScript/commits/spelling-full has one commit per word family, which I generally find easier to review.

Each group has its own preferred way to review such things. The approach I took w/ ChakraCore was different than this one.

@RyanCavanaugh
Copy link
Member

I think this set of groups should produce manageable diffs:

  • checker.ts
  • services.ts
  • parser.ts and binder.ts
  • Everything else under /src/compiler
  • Everything else under /src/
  • diagnosticMessages.json (do this after all the others so we don't see error message diffs in the other groups)
  • tests/baselines/* that are different due to diagnosticMessages.json

@DanielRosenwasser
Copy link
Member

To clarify, everything else under src/compiler that isn't generated. Also don't touch tests/baselines except by actually running the tests with jake runtests and jake baseline-accept.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 5, 2016

@RyanCavanaugh checker.ts is now #6932

I'm going to keep https://github.com/jsoref/TypeScript/commits/spelling-full up to date with the full series of changes, and I'll probably create a series for them for people to monitor (spelling-split).

I'll also strip out the tests/baselines from the patches next week (by moving them into a deletion commit "ignore").

Some repositories have pull-request hooks to automatically run commit tests and things, it was interesting to see that this one does not.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 9, 2016

services.ts is now #6988

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants