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

Added Markdown link and backslash escape support #24

Merged
merged 7 commits into from
Nov 8, 2017

Conversation

htinlinn
Copy link
Contributor

@htinlinn htinlinn commented Oct 27, 2017

The parser stack has been restructured and a new step has been introduced to allow for matching different opening and closing symbols. As a result, it's likely slower but way more flexible. Backslash escape support (issue #2) was also added to be able to parse URLs that might include parentheses in them.

@htinlinn htinlinn requested review from Xodia and ghvg1313 October 27, 2017 21:13
@htinlinn htinlinn force-pushed the feature/links-and-escaping branch from e3c9e63 to 5d08e3f Compare October 27, 2017 21:20
Copy link

@phillfarrugia phillfarrugia left a comment

Choose a reason for hiding this comment

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

Nice! I specifically like that you've improved the use of types like Rule, Token, Symbol to be more declarative of the behavior of different operations 😀 👌🏻

var elements: [MarkdownElement] = []

var i = 0
while i < tokens.count {

Choose a reason for hiding this comment

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

is there a specific reason you used a while loop as opposed to for (index, element) in <collection>.enumerated()?

i.e. this would remove the need to increment i each time

for (index, token) in tokens.enumerated() {
    // Process each token here
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using a while loop here because, in the case of parsing links, I am iterating through two tokens at once; the enumerated for loop wouldn't let me do the same.

/// Shorthand for `String.Index` used in parsing.
internal typealias Index = String.Index

internal extension String {

Choose a reason for hiding this comment

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

minor, but in some places you define extensions as explicitly internal, whereas in others you leave the protection level ommitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll update this throughout the app to be consistent.

// Copyright © 2017 Prolific Interactive. All rights reserved.
//

import Foundation

Choose a reason for hiding this comment

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

something minor I came across very recently actually, is in a file like this you're not actually using Foundation for anything (like NSObject, so this import is useless) you could actually remove it if you wanted to. I don't think there's any benefit other than conciseness though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I try remove it when I remember but, most of the time, I don't.

Copy link

@Xodia Xodia left a comment

Choose a reason for hiding this comment

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

Approving but some issue with links with less than 4 chars (eg: [HELLO](mail://))

Copy link

@ghvg1313 ghvg1313 left a comment

Choose a reason for hiding this comment

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

Simple logic, good job

@htinlinn
Copy link
Contributor Author

htinlinn commented Nov 8, 2017

@Xodia seems like the issue was only with a beta version of Xcode.

@htinlinn htinlinn merged commit 3cb8bd7 into master Nov 8, 2017
@htinlinn htinlinn deleted the feature/links-and-escaping branch November 8, 2017 15:17
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.

4 participants