-
Notifications
You must be signed in to change notification settings - Fork 709
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
#837 Add contributing.md #841
Conversation
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.
Thanks for this!
contributing.md
Outdated
|
||
#### Building | ||
|
||
To compile the TypeDoc source, run `npm grunt`. This will start the TypeScript compiler and output the compiled JavaScript to the `dist` folder. If you want to build and test in one step, run `npm build`. |
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.
Should be npm run grunt
and npm run build
?
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.
Yep 👍 Need the run
command.
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.
Fixed! That's what I get for using yarn...
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.
Great start! Thanks for putting this together.
@@ -0,0 +1,2 @@ | |||
# Disable core.autocrlf's line ending conversion behavior to prevent test failures on Windows | |||
* text=lf |
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.
Are you testing this on windows? I thought on windows you want git to checkout files with crlf so that it matches windows editors. Does the build produce crlf endings?
If going with lf, we probably want an .editorconfig
file with that setting as well.
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.
Yes, my current environment is Windows 10 + Linux Subsystem
In general, yes, on Windows you should checkout files with crlf, however the build will use whatever line endings files are checked out with, resulting in failing tests.
It might be a better idea to normalize line endings when doing output instead of this, thoughts?
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 practice, the CRLF situation leads to many issues across platforms. All Windows editors support LF properly so I found it easier to enforce LF.
I use a similar .gitattributes
(text eol=lf
) file and the following .editorconfig
setting:
end_of_line = lf
In tsconfig.json
, you should also ensure that "newLine": "lf"
is set.
contributing.md
Outdated
|
||
For instructions on setting up your environment, see the [setup](#setup---git-github-and-node) instructions in this document. | ||
|
||
If you have started work on an issue and get stuck or want a second opinion on your implementation feel free to reach out through [Gitter]. |
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 wondering if there is a way to make the gitter channel more visible. It is really helpful to iterate on ideas before a PR is submitted.
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've added the Gitter link to the PR section as well, I would have liked to put it under the "How Can I Contribute" section, but struggled to come up with appropriate wording.
This is a minor nitpick, but could you rename the file to uppercase ( |
* Sets newLine config
I really don't think this is complete - but its a start!
This PR adds contributing.md, adds a link to it in the readme, and rebuilds the specs to include the updated readme.
It also includes a
.gitattributes
file which overrides thecore.autocrlf = true
setting that most Windows Git setups will have. This is important because TypeDoc does not normalize line endings when parsing files. If a file is checked out with crlf line endings and aspecs.json
file is generated, tests can fail because multiline strings will include"\r\n"
instead of"\n"
. Most editors (Notepad being the one exception that comes to mind) will correctly handle either line endings, so this shouldn't cause any issues there.