Skip to content

incremental progress on #19 - populating the contributor's guide #73

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

Merged
merged 3 commits into from
May 20, 2021

Conversation

clyne
Copy link
Contributor

@clyne clyne commented May 12, 2021

A few minor clarifications. No substantiative changes

@clyne clyne requested a review from kmpaul as a code owner May 12, 2021 23:49
Copy link
Collaborator

@kmpaul kmpaul left a comment

Choose a reason for hiding this comment

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

Extremely minor change, but otherwise looks great. Thanks, @clyne!

README.md Outdated
Comment on lines 42 to 48
However, many of the links will not work. For all of the links found in the portal to work properly, you'll need to set up a local testing server. This can be done with Python's http.server by running the following command from within the `content` directory:

```
python -m http.server --directory _build/html/
```

and then pointing your browser at the URL: `localhost:8000`
Copy link
Member

Choose a reason for hiding this comment

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

It's my understanding that this step isn't needed since we provide a live directive in the Makefile for running the server:

live:
sphinx-autobuild --ignore _build -b dirhtml . _build/dirhtml/

I think this could be replaced by

make live

Copy link
Member

Choose a reason for hiding this comment

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

sphinx-autobuild has an added benefit of including a livereload enabled web server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andersy005 that would be cool. Have you tried it? When I run 'make live' I get into what looks like an infinite loop. I've tried blowing away the _build directory first.

Copy link
Member

Choose a reason for hiding this comment

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

When I run 'make live' I get into what looks like an infinite loop.

The make live command doesn't return because sphinx-autobuild keeps watching the directory for new changes (when you make changes to the source files, it rebuilds the website without you having to re-run the make live command) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Doesn't seem to be working for me: the links still aren't being resolved. Have you tried it? What is the order of operations you are using?

Copy link
Member

Choose a reason for hiding this comment

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

It's currently not working on my end. I suggest merging this PR as is, then I will look into this issue in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me if you want to resolve this comment and approve the PR. Thanks

Co-authored-by: Kevin Paul <[email protected]>
@clyne clyne requested a review from kmpaul May 18, 2021 23:33
Copy link
Collaborator

@kmpaul kmpaul left a comment

Choose a reason for hiding this comment

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

I think we should merge this.

Copy link
Contributor

@ktyle ktyle left a comment

Choose a reason for hiding this comment

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

Let's merge!

@clyne clyne requested a review from andersy005 May 20, 2021 15:54
@kmpaul kmpaul merged commit 81d1d51 into main May 20, 2021
@andersy005 andersy005 deleted the issue_19 branch June 11, 2021 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants