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.

Sign up for GitHub

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

Jump to bottom

url part of footnotes and links shall not be checked #102

Closed
wants to merge 10 commits into from
Closed

url part of footnotes and links shall not be checked #102

wants to merge 10 commits into from

Conversation

laysauchoa
Copy link
Collaborator

@laysauchoa laysauchoa commented Aug 24, 2020 •
edited
Loading

What does this PR accomplish?

Remove check of links, but still keep title of inline links being checked.

  • 🩹 Bug Fix

Closes #97

Changes proposed by this PR:

Notes to reviewer:

I have some questions/concerns about footnote handling.

📜 Checklist

  • Works on the ./demo sub directory
  • Test coverage is excellent and passes
  • Documentation is thorough

@laysauchoa laysauchoa changed the title #97 URL part of footnotes and links shall not be checked #97 🔗 URL part of footnotes and links shall not be checked - #97 Aug 24, 2020
@laysauchoa laysauchoa requested a review from drahnr August 24, 2020 21:44
@laysauchoa laysauchoa changed the title 🔗 URL part of footnotes and links shall not be checked - #97 [#97] 🔗 URL part of footnotes and links shall not be checked Aug 24, 2020
drahnr
drahnr reviewed Aug 25, 2020
View reviewed changes
src/documentation/markdown.rs Outdated
//Collapsed,
//CollapsedUnknown,
//Shortcut,
//ShortcutUnknown,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a test case for each of them, and then enable them one by one and see if special handling is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I agree. I will look into this and get back to you.

drahnr
drahnr reviewed Aug 25, 2020
View reviewed changes
src/documentation/markdown.rs Outdated
@@ -99,7 +131,8 @@ impl<'a> PlainOverlay<'a> {
}
}
Event::Text(s) => {
if code_block {
if code_block || skip_text {
skip_text = false
Copy link
Owner

@drahnr drahnr Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are nested links ok? If so, would this cause issues here?
I think skip_text is better placed in the End tag, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you write an example of a nested link? I can check it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. [link of [links](./a)](./b) (not sure this is legal, I did not check the spec)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it does not support nested links. There is also not option in the pulldown-cmark crate

drahnr
drahnr reviewed Aug 25, 2020
View reviewed changes
src/documentation/markdown.rs Outdated

#[test]
fn markdown_reduction_mapping_inline_link() {
const MARKDOWN: &str = r#" dyrck [I'm an inline-style link](https://www.google.com) artic"#;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const MARKDOWN: &str = r#" dyrck [I'm an inline-style link](https://www.google.com) artic"#;
const MARKDOWN: &str = r#" prefix [I am feeling ducky](https://duckduckgo.com) postfix"#;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix is not a misspelled word here

drahnr
drahnr reviewed Aug 25, 2020
View reviewed changes
Copy link
Owner

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are on the right track 👍 - see details inline

@drahnr drahnr changed the title [#97] 🔗 URL part of footnotes and links shall not be checked 🔗 URL part of footnotes and links shall not be checked Aug 25, 2020
@laysauchoa laysauchoa changed the title 🔗 URL part of footnotes and links shall not be checked url part of footnotes and links shall not be checked Aug 25, 2020
@laysauchoa @drahnr
fix/markdown: improve syntax and variable names
646a0c2 
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
@laysauchoa laysauchoa force-pushed the laysa-no-checks-for-links-and-footnotes branch from a63f0e9 to 646a0c2 Compare August 27, 2020 18:14
@laysauchoa laysauchoa force-pushed the laysa-no-checks-for-links-and-footnotes branch from 88d78a3 to 313c350 Compare August 27, 2020 18:38
laysauchoa
laysauchoa commented Aug 27, 2020
View reviewed changes
src/documentation/markdown.rs Outdated
// for now, only dealing with some links types
match link_type {
LinkType::Inline => {}
LinkType::Autolink | LinkType::Email => skip_link_text = true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually checked and I could not reproduce those types extra types. Either does not recognize them correct because they are not yet supported or I am getting the wrong examples to test (not sure, there are not many in the docs).

https://docs.rs/pulldown-cmark/0.7.2/pulldown_cmark/enum.LinkType.html

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate? I am not sure I can follow.

@laysauchoa
Copy link
Collaborator Author

laysauchoa commented Aug 28, 2020 •
edited
Loading

I have a small issue for footnote's since [^ ] are messing up with the mapping. Seems that I would need to add +2 in the beginning to count that. Otherwise, I have wrong marks.

I added some debugging statements if it helps. I want to check it with you if I should do that.

event: Start(Paragraph)
event: Text(Borrowed("Here\'s a simple footnote,and here\'s a longer one."))
event: FootnoteReference(Borrowed("bignote"))
event: Text(Borrowed(" something else "))
event: FootnoteReference(Borrowed("bignote"))
event: Text(Borrowed(": This is the first footnote."))
event: End(Paragraph)
Mapp {0..49: 1..50}
Mapp {0..49: 1..50, 49..56: 50..60, 56..72: 60..76}
error: spellcheck(Hunspell)
  --> /home/tmhdev/Documents/rust_samples/recursion_rust/src/fibonnacci.rs:1
   |
 1 |  Here's a simple footnote,and here's a longer one.[^bignote] something else [^bignote]: This is the first footnote.
   |                                                   ^^^^^^^
   | - big note, big-note, or mignonette
   |
   |   Possible spelling mistake found.

error: spellcheck(Hunspell)
  --> /home/tmhdev/Documents/rust_samples/recursion_rust/src/fibonnacci.rs:1
   |
 1 |  Here's a simple footnote,and here's a longer one.[^bignote] something else [^bignote]: This is the first footnote.
   |                                                                             ^^^^^^^
   | - big note, big-note, or mignonette
   |
   |   Possible spelling mistake found.

Following up for the other links, I was not able to generate the events for them, such as you can see here for FootnoteReference I could generate it. Let me know if it is clear ^^

@laysauchoa laysauchoa requested a review from drahnr August 28, 2020 15:59
@drahnr
Copy link
Owner

drahnr commented Aug 28, 2020

Could you add tests for all variants, even if they are not decided properly, that would at least be able to track changes in the cmark pulldown parser and avoid any fallout because of that.

If you do that, then I could peek into that tomorrow.

@laysauchoa
Copy link
Collaborator Author

Could you add tests for all variants, even if they are not decided properly, that would at least be able to track changes in the cmark pulldown parser and avoid any fallout because of that.

If you do that, then I could peek into that tomorrow.

I opened an issue about this as it is not clear for me what they are from the docs: pulldown-cmark/pulldown-cmark#472

@laysauchoa
Copy link
Collaborator Author

cc @drahnr

I forgot to push those changes yesterday 🤦🏻

4524ce7

@drahnr
Copy link
Owner

drahnr commented Aug 30, 2020

Since I added you as collaborator, you should be able to directly create branches here, so no need to fork anymore and I can just directly push adjustments to your branches :)

@drahnr drahnr mentioned this pull request Aug 30, 2020
Merged
3 tasks
@drahnr
Copy link
Owner

drahnr commented Aug 30, 2020 •
edited
Loading

There seems to be an issue with the link representation in cmark pulldown, I added comments here: pulldown-cmark/pulldown-cmark#472

I would suggest to continue dev in #105 and close this to ease the chores of rebase and refork and collaboration in general.

@laysauchoa
Copy link
Collaborator Author

Closes in favor of #105
Changes will be continued there.

@laysauchoa laysauchoa closed this Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drahnr drahnr Awaiting requested review from drahnr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

url part of footnotes and links shall not be checked
2 participants
@laysauchoa @drahnr