Skip to content
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

Updates to docs for website build #3649

Closed
wants to merge 4 commits into from
Closed

Updates to docs for website build #3649

wants to merge 4 commits into from

Conversation

bjascob
Copy link
Contributor

@bjascob bjascob commented Apr 26, 2019

Minor updates to documentation for building the website locally. This closes #3648.

Description

CONTRIBUTING.md contained instructions for the old harp web server. This was replaced with a reference to website/README.md. In that file the only change was to add a note that Node 10.15 or later is required.

Note: It was suggested by @ines to also add the node version (via the "engines" param) to the package.json file. While this seemed simple enough, unfortunately I was not able to get it to produce a warning message during npm install and so did not include the change in this PR.

Types of change

Simple doc change only

Checklist

  • I have submitted the spaCy Contributor Agreement.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@bjascob
Copy link
Contributor Author

bjascob commented Apr 26, 2019

Just FYI that the npm documentation for the "engines" param says "this field is advisory only and will only produce warnings when your package is installed as a dependency.". Looks like since spacy-io is the top-level app and not a dependency, it's not going to produce a warning.

@ines
Copy link
Member

ines commented Apr 26, 2019

@bjascob Thanks! And okay, I should have just read the npm docs 😅

@ines ines added the docs Documentation and website label Apr 26, 2019
Copy link
Member

@ines ines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you might have accidentally pushed this onto the same branch as your other PR in #3646? So this PR now includes both changes. Aside from that, the changes look good!

@bjascob
Copy link
Contributor Author

bjascob commented Apr 26, 2019

Sorry about that. I do have two separate branches but I must have pushed changes to both accidentally.

In order to fix this, from this branch I removed the test_issue3484.py and did a "git revert" on spacy/lemmatizer.py. Those files are not related to this PR. Not sure what will happen when you try to merge in these changes and the changes from PR #3646. I'm guessing you'll have a conflict and will need to choose to keep those 2 files from #3646. If that's an issue, let me know. We can just kill this entire PR/branch and I can resubmit from a clean master.

@bjascob bjascob closed this Apr 26, 2019
@bjascob bjascob deleted the doc-issue-3648 branch April 26, 2019 23:28
@bjascob bjascob mentioned this pull request Apr 26, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation and website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Website local build instructions
2 participants