-
Notifications
You must be signed in to change notification settings - Fork 158
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
Fixes for <exclude/> support, with tests and docs. #787
Conversation
Documentation on rebasing links Issues fsprojects#786 , fsprojects#785
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.
This looks like a solid PR. I have some minor nitpicks along the way but I think we can take this in.
At the same time, I'm no expert so @nhirschey do you want to glance over this one as well?
@davedawkins if you update the RELEASE_NOTES.md
file with your fix and a new version (say 17.2.1
) a new version would be released once we merged this to main.
If you don't need a new version that's fine as well.
Remove unnecessary comments as discussed in PR review
All updates made as requested. Thank you for the constructive feedback, and sorry for the typos and silly mistakes |
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.
Looks good to me.
Thanks so much @davedawkins. @nojaf sure, I should be able take a look in a few hours. |
Please hold off from applying this PR. I think I've found an issue with excluding |
There's no issue with excluding type as such, but the enclosing module will still list an excluded type. Additionally, the search index is listing excluded items, so I'm going to fix both those issues today. Unless requested otherwise, I'm going to check those changes into this branch (and hence this PR). |
Reinstate commented out tests :-/
Hello @davedawkins, thank you so much for your persistence here! I think having the index changes in this PR is ok. I took a peek at the latest changes and everything is still ok for me. |
@nojaf, no problem. I'm determined to get Sutil's API documented! Thank you for your help too. |
Wonderful set of fixes @davedawkins. Thank you! It looks good to me as well. I went ahead and merged the PR to trigger the release. It should hit the feed soon. |
Documentation on rebasing links
Issues #786 , #785