Skip to content

Conversation

phlax
Copy link
Contributor

@phlax phlax commented Oct 8, 2020

this allows devs to explain why something has been added, or remove after a certain amount of time

@Daltz333
Copy link
Collaborator

Daltz333 commented Oct 8, 2020

Can you add a test that reads a file with comments and verifies redirects are still added?

@phlax
Copy link
Contributor Author

phlax commented Oct 8, 2020

sure

@phlax
Copy link
Contributor Author

phlax commented Oct 8, 2020

ci seems to be failing for unrelated reasons tho i think

@phlax
Copy link
Contributor Author

phlax commented Oct 8, 2020

@Daltz333 iiuc just adding this sample in tests/root/ext should test - can you confirm ? I dont think i can see CI as its falling over

sorry, read tests properly adding test now...

@TheTripleV
Copy link
Collaborator

CI is operational again. Could you rebase off of master to get the fix?

@phlax phlax force-pushed the comments-in-redirects-file branch from fddfd4a to 76409c2 Compare October 11, 2020 04:06
Copy link
Collaborator

@TheTripleV TheTripleV left a comment

Choose a reason for hiding this comment

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

Since this pr changes functionality in the create_graph function, tests should go in tests/test_create_graph.py.

I'd say the only test required is:
rediraffe.txt has a line with a comment and that line should not appear in the output of create_graph.

@phlax phlax force-pushed the comments-in-redirects-file branch from 76409c2 to fa9161f Compare October 11, 2020 07:33
@phlax
Copy link
Contributor Author

phlax commented Oct 13, 2020

@TheTripleV i think ive moved the test to the right place, lmk if there are any further changes required to land

Copy link
Collaborator

@TheTripleV TheTripleV left a comment

Choose a reason for hiding this comment

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

LGTM!

@Daltz333 Daltz333 merged commit 3378157 into sphinx-doc:master Oct 13, 2020
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.

3 participants