fix: Allow a semiliterate destination without a matching group in pattern #94
Conversation
…tern Prior to this commit, the simple plugin would crash if the pattern had no matching group. But that usage should be allowed if there is a "destination" parameter that does not depend on a matching group, as in the case of a file being renamed into the doc site. This commit fixes the issue, and also takes the liberty of slightly streamlining the invocation of the extraction process for ease of inheriting from the simple plugin. It also adds a test case for a semiliterate block with a `destination` parameter and no matching group in its `pattern` parameter.
gwhitney
force-pushed
the
allow_rename
branch
from
3f974b8
to
7de9e79
Compare
|
Oops forgot one small point: since extraction from a file can produce unexpected results, I've found it is more useful in tracking what's going on to issue a debug message when scanning starts and then when extraction is complete, rather than waiting for extraction to complete to generate any diagnostic. I slipped that two-line change in as well just now, hope that's helpful. |
mkdocs_simple_plugin/plugin.py
Outdated
| "mkdocs-simple-plugin: No last group in match of" + | ||
| "{} to {} and no destination".format( | ||
| item['pattern'], name)) | ||
| new_name = (name[:name_match.start(name_match.lastindex)] + |
There was a problem hiding this comment.
I think it would be clearer if this was something like
new_name = os.path.splitext(name)[0]+'.md'If someone wants to change the filename to something other than the same file with '.md' extension, they should probably just use the destination field.
There was a problem hiding this comment.
Seems reasonable to me, and that will eliminate the need for there to be a matching group in the pattern when there's no definition. I will change to your suggested behavior and update the docs.
There was a problem hiding this comment.
I also added descriptions of the behaviors of the default semiliterate blocks.
As suggested by mkdocs-simple-plugin maintainer. Also need to update test-mkdocs.yml (to come). Co-authored-by: Allison Thackston <allison@allisonthackston.com>
|
OK, have responded to three comments, will be happy to update the PR when I get your reply to the first item. |
|
Huh, wait -- I am confused: how did test.bats pass with a {% %} directive but no macros plugin in test-mkdocs.yml? Is that something I should look into, or do you understand why it's OK? Thanks. |
|
Oh, if you don't include it as a plugin it just copies the text verbatim (ie, it doesn't extract). It's fine for this test, since macro inclusion isn't really the purpose, and it will get generated appropriately for the documentation site. |
|
One thing I noticed as I was executing your suggested change to how extensions of extracted files work: in the default semiliterate parameters for dockerfiles and YAML, you now have: Because of that Just wanted to make sure you were aware of the new optionality of the comment symbol, and ask if you'd like me to update the comment on this as suggested. (or of course, feel free to do it yourself before merging, but I am happy to do it if you'd like) |
As suggested in code review. Also update documentation to reflect this change, and add description of the behavior of the default semiliterate blocks.
|
OK, that should do it, modulo if you want me to do anything about the comment on the YAML |
There was a problem hiding this comment.
Thanks!
It's probably not worth updating the wording for that comment. I've run into some issues where the block style comments and the inline-style comments can't be interleaved. I tried to put both patterns in one match, but then the h1 heading is getting stripped from the block style comments. I've hacked around it now by using === but it should probably be fixed for real before I make a new release.
| # * From C, C++, and Javascript files: block comments | ||
| # starting with `/** md`. | ||
| # /md | ||
| 'pattern': r'\.(cpp|cc?|h|hpp|js)$', |
Prior to this commit, the simple plugin would crash if the pattern
had no matching group. But that usage should be allowed if there is
a "destination" parameter that does not depend on a matching group,
as in the case of a file being renamed into the doc site.
This commit fixes the issue, and also takes the liberty of slightly
streamlining the invocation of the extraction process for ease of
inheriting from the simple plugin.
It also adds a test case for a semiliterate block with a
destinationparameter and no matching group in its
patternparameter.Resolves #93.