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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log kramdown warnings if log level is WARN #6522

Merged
merged 2 commits into from Nov 9, 2017

Conversation

Projects
None yet
7 participants
@Crunch09
Member

Crunch09 commented Nov 5, 2017

This fixes #6516 . Thank you @gettalong for making us aware of this option! 馃憤

cc: @jekyll/build

@jekyllbot jekyllbot requested review from ayastreb, mattr- and parkr Nov 5, 2017

@@ -29,7 +29,12 @@ def output_ext(_)
end
def convert(content)
Kramdown::Document.new(content, @config).to_html.chomp
document = Kramdown::Document.new(content, @config)
html_output = document.to_html.chomp

This comment has been minimized.

@Crunch09

Crunch09 Nov 5, 2017

Member

Is there any reason why we're doing chomp here but not in KramdownParser above?

@Crunch09

Crunch09 Nov 5, 2017

Member

Is there any reason why we're doing chomp here but not in KramdownParser above?

This comment has been minimized.

@pathawks

pathawks Nov 6, 2017

Member

My thinking was that SmartyPants only deals with inline elements (and not block elements), so white space is insignificant.

@pathawks

pathawks Nov 6, 2017

Member

My thinking was that SmartyPants only deals with inline elements (and not block elements), so white space is insignificant.

@DirtyF

Why not use Jekyll.logger.warn here?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 5, 2017

Member

Why not use Jekyll.logger.warn here?

Uhm, good point 馃槅 There's no real reason for choosing debug

Member

Crunch09 commented Nov 5, 2017

Why not use Jekyll.logger.warn here?

Uhm, good point 馃槅 There's no real reason for choosing debug

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 6, 2017

Member

@Crunch09, in #6516 Gettalong mentioned activating the warning channel with a configuration option.. IMO, that is a nice idea, because, then the user can be issued Jekyll.logger.warn messages which will be output without all the clutter from the --verbose option..

Member

ashmaroli commented Nov 6, 2017

@Crunch09, in #6516 Gettalong mentioned activating the warning channel with a configuration option.. IMO, that is a nice idea, because, then the user can be issued Jekyll.logger.warn messages which will be output without all the clutter from the --verbose option..

@ccorn ccorn referenced this pull request Nov 6, 2017

Closed

New math engine SsKaTeX #455

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 6, 2017

Member

@ashmaroli @DirtyF I agree with you, changed to #warn now.

Member

Crunch09 commented Nov 6, 2017

@ashmaroli @DirtyF I agree with you, changed to #warn now.

@Crunch09 Crunch09 changed the title from Log kramdown warnings if log level is DEBUG to Log kramdown warnings if log level is WARN Nov 6, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 7, 2017

Member

changed to #warn now.

and the configuration option..? Is that for a future PR?

Member

ashmaroli commented Nov 7, 2017

changed to #warn now.

and the configuration option..? Is that for a future PR?

@DirtyF DirtyF added this to the v3.7.0 milestone Nov 7, 2017

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 7, 2017

Member

For example, kramdown warns when it finds duplicate reference links being declared or when a reference-style inline link refers to a missing link ID.

These are useful warnings. They should be displayed by default.

People can still use the --quiet option if they don't want any output.

Member

DirtyF commented Nov 7, 2017

For example, kramdown warns when it finds duplicate reference links being declared or when a reference-style inline link refers to a missing link ID.

These are useful warnings. They should be displayed by default.

People can still use the --quiet option if they don't want any output.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 7, 2017

Member

These are useful warnings. They should be displayed by default.

Fair enough. 馃憤

Member

ashmaroli commented Nov 7, 2017

These are useful warnings. They should be displayed by default.

Fair enough. 馃憤

@oe

oe approved these changes Nov 9, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 9, 2017

Member

It would also be great to see if this could be done for other converters.

@jekyllbot: merge +minor

Member

pathawks commented Nov 9, 2017

It would also be great to see if this could be done for other converters.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit fa22ebf into jekyll:master Nov 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 10, 2017

Member

Here are the errors on Jekyll's website, the path of the file is missing:

 Kramdown warning: No link definition for link ID '#1339' found on line 2
  Kramdown warning: No link definition for link ID '' found on line 2
  Kramdown warning: No link definition for link ID '#1338' found on line 3
  Kramdown warning: No link definition for link ID '' found on line 3
  Kramdown warning: No link definition for link ID 'docs' found on line 150
  Kramdown warning: No link definition for link ID 'rubocop' found on line 260
  Kramdown warning: No link definition for link ID '"permalink"' found on line 299
  Kramdown warning: No link definition for link ID 'site' found on line 375
  Kramdown warning: No link definition for link ID 'docs' found on line 437
  Kramdown warning: No link definition for link ID 'docs' found on line 580
  Kramdown warning: No link definition for link ID 'docs' found on line 583
  Kramdown warning: No link definition for link ID 'docs' found on line 592
  Kramdown warning: No link definition for link ID 'add note' found on line 926
  Kramdown warning: No link definition for link ID '' found on line 1002
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
Member

DirtyF commented Nov 10, 2017

Here are the errors on Jekyll's website, the path of the file is missing:

 Kramdown warning: No link definition for link ID '#1339' found on line 2
  Kramdown warning: No link definition for link ID '' found on line 2
  Kramdown warning: No link definition for link ID '#1338' found on line 3
  Kramdown warning: No link definition for link ID '' found on line 3
  Kramdown warning: No link definition for link ID 'docs' found on line 150
  Kramdown warning: No link definition for link ID 'rubocop' found on line 260
  Kramdown warning: No link definition for link ID '"permalink"' found on line 299
  Kramdown warning: No link definition for link ID 'site' found on line 375
  Kramdown warning: No link definition for link ID 'docs' found on line 437
  Kramdown warning: No link definition for link ID 'docs' found on line 580
  Kramdown warning: No link definition for link ID 'docs' found on line 583
  Kramdown warning: No link definition for link ID 'docs' found on line 592
  Kramdown warning: No link definition for link ID 'add note' found on line 926
  Kramdown warning: No link definition for link ID '' found on line 1002
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
  Kramdown warning: Warning: this should not occur - no block parser handled the line
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 10, 2017

Member

@DirtyF Could you open a new issue, that we should add the file path to these warnings? This feature will be super helpful!

Member

pathawks commented Nov 10, 2017

@DirtyF Could you open a new issue, that we should add the file path to these warnings? This feature will be super helpful!

@Crunch09 Crunch09 deleted the Crunch09:log_kramdown_warnings branch Nov 12, 2017

document = Kramdown::Document.new(content, @config)
html_output = document.to_html.chomp
document.warnings.each do |warning|
Jekyll.logger.warn "Kramdown warning:", warning

This comment has been minimized.

@pathawks

pathawks Nov 14, 2017

Member

It looks like there are currently no (legitimate) warnings raised by any of the parsers SmartyPants is using in kramdown. Perhaps we just rip this out?

@pathawks

pathawks Nov 14, 2017

Member

It looks like there are currently no (legitimate) warnings raised by any of the parsers SmartyPants is using in kramdown. Perhaps we just rip this out?

This comment has been minimized.

@Crunch09

Crunch09 Nov 18, 2017

Member

Apologies for the late response. No objections to remove it, if it doesn't help in any way

@Crunch09

Crunch09 Nov 18, 2017

Member

Apologies for the late response. No objections to remove it, if it doesn't help in any way

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