Handle TracWiki PageOutline directive

[danuker]

Scope

This is the easier part of ticket #5 .
Ensure that the RST .. contents:: directive exists on every page, and remove all TracWiki [[PageOutline]] directives.

Changes

The changes were implemented by removing all [[PageOutline]] mentions, and then adding RST .. contents:: to every page that is missing it.

Several places had the RST directive with an extra space; they are handled.

Also, address pytest warnings about escaping by using raw strings when needed for regexes.

Also, address a comment I missed about removing pytest args from the GitHub Action, now that there is a setup.cfg.

What is not done: The harder part of #5: replacing TracWiki [[TitleIndex]] directives with a list of RST links in a re-runnable and update-able way.

How to try and test the changes

reviewers: @adiroiban

Check that all files have a mention of .. contents::::

grep -rl . | egrep "rst$|rest$" | sort > all.txt
grep -rl  "\.\. contents::"  | grep -v "\.git" | sort > contents.txt
meld all.txt contents.txt

Meld (or your diff tool of choice) should show the same RST files inside.

Check that no files have more than one instance of .. contents::: the following command should not return anything:

 egrep -rc  "\.\.(\ )+contents::"  . | grep -v ":[01]$"

Also, I looked at the diff actual output of the script, and tried out changes in GitHub edit preview::

python wiki_trac_rst_convert.py /path/to/server.wiki

[danuker]

needs-review

Would appreciate a look!

[adiroiban]

It looks like the right direction, but it needs a bit more work on the testing side.

When implementing a feature try to make a list of low-level requirement to keep you focused on what you should implement. Use the list for writing tests.

If the implementation has any side-effect that don't conflict with the requirements, you don't have to bother writing tests for that side-effect behavior.

---

Note that the list of requirements is just an example.
You can decide to remove the requirement for handling multiple existing TOC directives, as we don't have that in practice.
Also, you can decide that if a page already has at least one RST local TOC directive, the page is kept as it is.

You can simplify the requirements.

The important part is to have the requirements and write a test for each requirement.

---

Thanks for the update. Looking forward to the re-review :)

[adiroiban]

Suggested change

	Make sure no TracWiki [[PageOutline]] directives remain.
	It removes any TracWiki [[PageOutline]] directives and replaced it with a single top of the page local TOC.

[danuker]

I opted for the docstring from the list below:

Pages that have one or more TracWiki local TOC (PageOutline)
will have them removed and replaced with a single RST local TOC.

[adiroiban]

Maybe add more, to make sure that it will not remove only the first one.

Suggested change

	'Sample content'
	)
	'[[PageOutline]]\n'
	'Sample content'
	'[[PageOutline]]\n'
	)

[adiroiban]

what is the difference between the previous assertion?

[danuker]

There is an extra space - I found some files where there are 2 spaces instead of 1 between the .. and the contents::. I will split the test case to clarify.

[adiroiban]

I think that here we need multiple tests as I see more requirements.
You can think of a test as a requirement validator.
And then you test docstring can be written as a requirement.

I see these requirements:

• Pages without RST or TracWiki local TOC markup will have a single RST local TOC inserted at the start of the page, before the first section.
• Pages that already have a RST local TOC at the start of the page are not changed.
• Pages that already have a RST local TOC inside the content (not at the top of the page), or have multiple local TOCs, will have them removed and a new local TOC is inserted at the start of the page.
• Pages that have one or more TracWiki local TOC (PageOutline) will have them removed and replaced with a single RST local TOC.

---

You can take each requirement and copy it as the docstring for a test.
Try to write a single test first, run it and see it fail and then update the code.
Then continue with the next test, see it fail, fix the code...etc

[adiroiban]

Here a prefer to see a normal assertEqual as otherwise the assertion is confusing in this context.

But you can observe that the assertConvertedContent did its job here, as you only had to update the assertion once, and all the existing content conversion tests didn't require any update.

assertConvertedContent should only be used for content convertion... the loca TOC is not really content, it's more page meta-content.

[danuker]

I agree. The tradeoff between assertEqual and assertConvertedContent is in readability vs. maintainability; and the TOC tests needed more readability.

[adiroiban]

Minor comment.
I bit of extra verbosity will not hurt, and the is should hint that this returns a boolean, like _is_path_allowed ... or even better:

Suggested change

	if _path_allowed(path):
	if _is_rst_file(path):

[adiroiban]

maybe do case insensitive matching

Suggested change

	return path.endswith('rst') or path.endswith('rest')
	path = path.lower()
	return path.endswith('rst') or path.endswith('rest')

[adiroiban]

maybe :

Suggested change

	Only work on actual rst and rest documents.
	Returns `True` if path looks like a ReStructuredText file.

The docstring should hint how and why to use it.

It shouldn't be a human readable version of the implementation :)

Also, "only work" is a vague ... what does that mean ?

I understand how you ended up with this docstring...but the doctring is not ok.

This is a valid comment for the if _is_path_allowed(path): but with a good name, we shouldn't need a comment.

[danuker]

needs-review

Handling "multiple local TOCs" test actually simplified the code! I could remove an "if" branch. Thank you!

[adiroiban]

Great work. Thanks! I have applied to the wiki.