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

WIP: Refactor markdown #174

Closed
wants to merge 21 commits into from

Conversation

RadicalZephyr
Copy link
Contributor

Here's our start at rewriting the markdown extension as proposed in #160. Feedback or suggestions are welcome!

Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

Looks like a good start!

parser: Peekable<Parser<'b>>,
}

fn is_internal_link(link: &str) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is only used once so I don't think it needs to be a function

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's only used once, but I think the value here is being explicit about what the parsing is looking for. When simply using link.starts_with("./") I think it's not as apparent that what we're really looking for is an internal link.

fn new(context: &'a Context<'a>, parser: Parser<'b>, errors: &'c mut Vec<Error>) -> GutenbergFlavoredMarkdownParser<'a, 'b, 'c> {
GutenbergFlavoredMarkdownParser {
context: context,
errors: errors,
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to repeat context and errors, {context, errors, parser: parser.peekable()} will work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoa! I had no idea! that's awesome

self.empty_event()
}
}
} else{
Copy link
Collaborator

Choose a reason for hiding this comment

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

space after else

}
}
} else{
Event::Start(Tag::Link(href.clone(), text.clone()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

not possible to return the link without cloning?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current implementation clones as well, so we just left it while it's still a WIP, but we'll go back at the end and clean up as many clones as possible.

}
}

#[allow(unused_variables)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

is that just while it's a wip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, to make fewer warnings. We'll make sure to remove these and fix warnings before finally submitting.

@Keats
Copy link
Collaborator

Keats commented Nov 14, 2017

Ah can you do the PR on the next branch by any chance?

@Keats
Copy link
Collaborator

Keats commented Dec 6, 2017

I'm guessing this PR can be redone from scratch with the coming changes to the shortcode handling? Or at least significant changes

@RadicalZephyr
Copy link
Contributor Author

Closing this for now, since we're going to regroup a bit on our approach. We'll open a new PR once we've rebased against the next branch.

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