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 an option to configure kramdown warning output #6554

Merged
merged 2 commits into from Dec 7, 2017

Conversation

Projects
None yet
4 participants
@ashmaroli
Member

ashmaroli commented Nov 13, 2017

With this, warnings from kramdown are swallowed by default.
Add log_warnings: true under kramdown: options to enable them

@ashmaroli ashmaroli requested review from DirtyF, pathawks and Crunch09 Nov 13, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 13, 2017

Member

I’m wondering if this should be a more general option (rather than kramdown specific) so that other converters can opt-in as well.

Member

pathawks commented Nov 13, 2017

I’m wondering if this should be a more general option (rather than kramdown specific) so that other converters can opt-in as well.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 13, 2017

Member

the warnings are from Kramdown.. why would other converters need to opt-in?

Member

ashmaroli commented Nov 13, 2017

the warnings are from Kramdown.. why would other converters need to opt-in?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 13, 2017

Member

@pathawks you're thinking of something like:

markdown: commonmark
  show_warnings: true

?

Member

DirtyF commented Nov 13, 2017

@pathawks you're thinking of something like:

markdown: commonmark
  show_warnings: true

?

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 13, 2017

Member

What I mean is an option that allows converters to output warnings, rather than sn option that allows kramdown to output warnings.

@DirtyF Yes, I’m thinking about CommonMark, but maybe Textile or SmartyPants also.

Member

pathawks commented Nov 13, 2017

What I mean is an option that allows converters to output warnings, rather than sn option that allows kramdown to output warnings.

@DirtyF Yes, I’m thinking about CommonMark, but maybe Textile or SmartyPants also.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 13, 2017

Member

changing config["markdown"] from a String to a Hash and then adding in code for validating and backwards-compatibility seems unnecessary complexity to me..

Member

ashmaroli commented Nov 13, 2017

changing config["markdown"] from a String to a Hash and then adding in code for validating and backwards-compatibility seems unnecessary complexity to me..

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 2, 2017

Member

We have show_drafts: true and —drafts. I’d be down to see show_warnings: true and —warnings.

Member

parkr commented Dec 2, 2017

We have show_drafts: true and —drafts. I’d be down to see show_warnings: true and —warnings.

@DirtyF DirtyF added this to the v3.7.0 milestone Dec 2, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Dec 3, 2017

Member

I’d be down to see show_warnings: true and --warnings.

hmm.. can I change them to show_converter_warnings: true and --converter-warnings? I agree the strings are longer but unlike verbose: true and --verbose that affects verbose output throughout the build, the show_warnings: false would only turn off warnings from converters. Other instances of Jekyll.logger.warn will still be fired unchecked..

Member

ashmaroli commented Dec 3, 2017

I’d be down to see show_warnings: true and --warnings.

hmm.. can I change them to show_converter_warnings: true and --converter-warnings? I agree the strings are longer but unlike verbose: true and --verbose that affects verbose output throughout the build, the show_warnings: false would only turn off warnings from converters. Other instances of Jekyll.logger.warn will still be fired unchecked..

@DirtyF

DirtyF approved these changes Dec 7, 2017

@DirtyF DirtyF merged commit fcb1b41 into jekyll:master Dec 7, 2017

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:kramdown-warning-config branch Dec 8, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Dec 8, 2017

Member

Why was this merged manually..? This needs to be registered in our History.markdown..

Member

ashmaroli commented Dec 8, 2017

Why was this merged manually..? This needs to be registered in our History.markdown..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment