-
Notifications
You must be signed in to change notification settings - Fork 876
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
Conversation
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. |
Concerning Release Notes - forgot those, will add that in a minute. Concerning tests - from the Contributing Guide:
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? |
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. |
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 Note that the existing tests for the TOC extension are in tests/extensions (in various files with names starting with |
Uh, I have zero experience in testing and have no clue what those files do. Maybe you could add those for me? Sorry :( |
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. |
As this was on a |
Fixes #781.