Skip to content
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

Add CHANGES_AST switch to plugin API so that AST breaking plugins can disable the equality check #49

Merged
merged 2 commits into from
Oct 3, 2020

Conversation

hukkin
Copy link
Owner

@hukkin hukkin commented Oct 1, 2020

No description provided.

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #49 into master will increase coverage by 0.47%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   96.53%   97.01%   +0.47%     
==========================================
  Files           9        9              
  Lines         462      469       +7     
==========================================
+ Hits          446      455       +9     
+ Misses         16       14       -2     
Impacted Files Coverage Δ
mdformat/_cli.py 97.22% <100.00%> (+3.01%) ⬆️
mdformat/plugins.py 92.59% <100.00%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2730892...d5a3bd8. Read the comment docs.

@hukkin
Copy link
Owner Author

hukkin commented Oct 1, 2020

Ping @chrisjsewell you might wanna have a look at this one?

@chrisjsewell
Copy link
Contributor

chrisjsewell commented Oct 2, 2020

Heya, can we make this a core CLI option? i.e. rather than hard-code the initial value of changes_ast = False, have something like:

parser.add_argument(
        "--skip-validation", action="store_true", help="Do not ensure the output Markdown produces the same HTML.")
...
changes_ast = args.skip_validation

This would be very useful for debugging issues.
Just the other day, I'd made a mistake in updating mdformat-tables, and so was just getting the message "The formatted Markdown renders to different HTML than the input Markdown.", which is not particularly helpful.
Having this option, it would have been very easy to quickly see what the problem was.

You could even call the option something like --debug

@hukkin
Copy link
Owner Author

hukkin commented Oct 3, 2020

Sure, I made an issue #50

I'm not a huge fan of adding functionality for developers in general (the tool is intended for users), but I think this could be possibly be useful for users as well.

@hukkin hukkin merged commit 58731b4 into master Oct 3, 2020
@hukkin hukkin deleted the changes-ast-plugin-api branch October 3, 2020 02:23
@chrisjsewell
Copy link
Contributor

I'm not a huge fan of adding functionality for developers in general (the tool is intended for users)

You realise this is to help you out as well. The rest of that error message reads:

The formatted Markdown renders to different HTML than the input Markdown.
This is likely a bug in mdformat. Please create an issue report here:
https://github.com/executablebooks/mdformat/issues

So basically, anytime a plugin has a bug like this, there is going to be a bunch of people opening an issue here, quoting this message, and you will have no easy way to debug it.

@hukkin
Copy link
Owner Author

hukkin commented Oct 3, 2020

Sure thing, but if they can provide the input, it should be very simple to reproduce (might be a good idea to add a note about that in that print).

@chrisjsewell
Copy link
Contributor

Sure thing, but if they can provide the input, it should be very simple to reproduce (might be a good idea to add a note about that in that print).

Well sure you will be able to reproduce the error message, how is that going to help you debug it?

Comparing to black: that provides the actual diff in the error message: https://github.com/psf/black/blob/283d999c3f89e2204cbf5a61242665bedd6887ef/src/black/__init__.py#L6258-L6260, and also provides the --fast argument (which I presume is why you suggest it in #50) https://github.com/psf/black/blob/283d999c3f89e2204cbf5a61242665bedd6887ef/src/black/__init__.py#L423-L427

@hukkin
Copy link
Owner Author

hukkin commented Oct 3, 2020

Well sure you will be able to reproduce the error message, how is that going to help you debug it?

With a debugger 😄

Comparing to black: that provides the actual diff in the error message:

Yeah adding the diff would be even better.

@hukkin
Copy link
Owner Author

hukkin commented Oct 3, 2020

Btw, totally unrelated, lol, but I got a first working build of the toc generator plugin https://github.com/hukkinj1/mdformat-toc

Tried it out on the README here https://github.com/hukkinj1/typenv and seems to be working fine, though I don't fully trust it yet (enough to add a link on mdformat README for instance).

😄 Thought you might be interested

@chrisjsewell
Copy link
Contributor

yeh looks good 👍

The key part of this though is the slugify function, which is unfortunately not standardised across all renderers.
Currently, you only supply one slugify functions, which is not documented where it came from or what render it can be used with.

As suggested before, It would be ideal to either integrate this with:
https://github.com/executablebooks/markdown-it-py/blob/master/markdown_it/extensions/anchors/index.py, or provide a separate, shared package that provided some slugify functions for the different renderers.

Examples of integration with markdown-it-anchor are https://www.npmjs.com/package/markdown-it-table-of-contents and https://www.npmjs.com/package/markdown-it-toc-done-right

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