Skip to content

Added permalink_title option #877

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

Closed
wants to merge 4 commits into from
Closed

Added permalink_title option #877

wants to merge 4 commits into from

Conversation

wilhelmer
Copy link

Fixes #781.

@waylan
Copy link
Member

waylan commented Nov 12, 2019

Look good. As noted in the Contributing Guide, this needs tests and an update to the Release Notes. Add those and I'll merge it.

@waylan waylan added the requires-changes Awaiting updates after a review. label Nov 12, 2019
@wilhelmer
Copy link
Author

Concerning Release Notes - forgot those, will add that in a minute.

Concerning tests - from the Contributing Guide:

While it is generally fine to leave those tests for the Travis server to run when a pull request is submitted, for more advanced changes, you may want to run those tests locally.

I don't consider this as an advanced change, and the tests on the Travis server have all passed, so which other tests are needed?

@waylan
Copy link
Member

waylan commented Nov 12, 2019

You need to add tests which confirm that the new option works as it is supposed to. Does it actually change the title when set? Does the default actually get used when not set?

I haven't checked, but the second question above might already get answered in the existing tests. If so, you would only need to add at least one new test. If not, then you would need to add at least two.

@waylan
Copy link
Member

waylan commented Nov 12, 2019

Note that as per the tests policy, any new tests should go in /tests/test_syntax/extensions. As none of the TOC tests have been migrated to the new system, you'll need to add a new file test_toc.py there and create your tests. At a minimum, you should create the two tests which confirm the default and a custom title get used.

Note that the existing tests for the TOC extension are in tests/extensions (in various files with names starting with toc) and the TestTOC class of tests/test_extensions.py. If any of those tests cover the same behavior, they should be converted to the new style tests and removed from here. I'm thinking perhaps the testPermalink tests in the TestTOC class. You don't have to covert any other tests, but are certainly welcome to.

@wilhelmer
Copy link
Author

wilhelmer commented Nov 12, 2019

Uh, I have zero experience in testing and have no clue what those files do. Maybe you could add those for me? Sorry :(

@waylan
Copy link
Member

waylan commented Nov 12, 2019

That's fine. However, I have very little time to do it. So unless someone else wants to do the work it will likely be some time before I get to it. I am not going to merge this (or anything else) without the tests.

@waylan
Copy link
Member

waylan commented Nov 26, 2019

As this was on a master branch, it was easier to just create a new PR than try to push changes to this. Therefore, this is being closed in favor of #886 which uses this as a base, resolves the conflicts and adds a test.

@waylan waylan closed this Nov 26, 2019
@waylan waylan added rejected The pull request is rejected for the stated reasons. and removed requires-changes Awaiting updates after a review. labels Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rejected The pull request is rejected for the stated reasons.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TOC: Configurable permalink title attribute
2 participants