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

Add resolver docs #8493

Merged
merged 2 commits into from
Jul 9, 2020
Merged

Add resolver docs #8493

merged 2 commits into from
Jul 9, 2020

Conversation

nlhkabu
Copy link
Member

@nlhkabu nlhkabu commented Jun 24, 2020

Closes #8459

TODO:

  • Build and preview
  • Add NEWS

docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
@pfmoore
Copy link
Member

pfmoore commented Jun 24, 2020

I'm not enough of a ReST expert to comment on the markup, but the content looks good to me.

@pradyunsg
Copy link
Member

pradyunsg commented Jun 24, 2020

@nlhkabu Pull Requests on pip's tracker automatically get their documentation built + made available for preview. It's a check called "docs/readthedocs.org:pip" in the list of checks on the PR, and "details" link on it, points to the preview.

The section added in this PR is available at: https://pip--8493.org.readthedocs.build/en/8493/user_guide/#fixing-conflicting-dependencies

Any changes made on this PR will also get reflected on that page. :)

@pfmoore
Copy link
Member

pfmoore commented Jun 24, 2020

TIL! :-)

Copy link
Contributor

@brainwane brainwane left a comment

Choose a reason for hiding this comment

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

This is a great document -- thank you for putting it together! I like the structure and the content and the clear writing with concrete examples.

docs/html/user_guide.rst Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
@deveshks
Copy link
Contributor

Hi @nlhkabu

Just as a suggestion, you can also add multiple review suggestions to a batch, and apply all of them under a single commit, instead of one commit per suggestion.

This is also described in the 2nd sub point of the 3rd point at https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/incorporating-feedback-in-your-pull-request

To add the suggestion to a batch of changes, click Add suggestion to batch. 
Continue to add the suggested changes you want to include in a single commit.
When you've finished adding suggested changes, click Commit suggestions.

@nlhkabu
Copy link
Member Author

nlhkabu commented Jun 26, 2020

Hi @deveshks - yes, I saw that, but unfortunately the "Add suggestion to batch" button was disabled on GitHub. Not sure if it has something to do with my permissions on this project.

I will squash the commits to clean up the commit history before finalising the PR.

@pradyunsg
Copy link
Member

@nlhkabu for batch suggestions, you need to go into the files view. (the disabled button in the other view has a tooltip indicating this)

@pradyunsg
Copy link
Member

I think this is good to go. Let's go ahead and merge this now -- we can iterate on this in the future as needed.

To that end, @nlhkabu could you please rebase this on master (to resolver the Travis CI failures) and add a news/8493.doc file containing a changelog entry (to satisfy the news file check)? :)

@nlhkabu
Copy link
Member Author

nlhkabu commented Jun 30, 2020

Yep @pradyunsg just making some edits on my side (both formatting and content). Hope to submit for merge today.

@nlhkabu nlhkabu force-pushed the add-resolver-docs branch 7 times, most recently from 519381e to b77670e Compare June 30, 2020 19:06
@nlhkabu
Copy link
Member Author

nlhkabu commented Jun 30, 2020

This is now ready for final review: https://pip--8493.org.readthedocs.build/en/8493/user_guide/#fixing-conflicting-dependencies

If we're happy with this, I'll add NEWS and clean up the commit history.

Note: I didn't convert the table to a definition list, as I was not sure where to put the examples in that format.

Thanks!

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Happy to merge once a NEWS entry is added! ^>^

@pradyunsg pradyunsg added this to the 20.2 milestone Jul 3, 2020
@nlhkabu nlhkabu marked this pull request as ready for review July 7, 2020 10:20
@nlhkabu
Copy link
Member Author

nlhkabu commented Jul 7, 2020

@pradyunsg this is ready to go on my side.
Not sure why "experimental" CI is failing?

@McSinyx
Copy link
Contributor

McSinyx commented Jul 7, 2020

@nlhkabu, the commit you're basing this PR on has them failed to. I'm not sure if that's necessary, but to get rid of the red cross you can rebase it on current master which is green.

@pradyunsg
Copy link
Member

@McSinyx's analysis is correct, but the failing checks aren't a blocker for merging this PR. I can merge it once I'm on my PC (GitHub doesn't let me merge from a mobile device if any CI check fails).

@nlhkabu
Copy link
Member Author

nlhkabu commented Jul 8, 2020

ping @pradyunsg for merge :)

@pradyunsg pradyunsg merged commit 6679b5e into master Jul 9, 2020
@pradyunsg pradyunsg deleted the add-resolver-docs branch July 9, 2020 20:32
@pradyunsg
Copy link
Member

Thanks for the ping @nlhkabu! ^>^

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2021
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.

Write documentation for ResolutionImpossible error message
7 participants