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

Removes extraneous newlines. #22

Merged
merged 5 commits into from Sep 5, 2016
Merged

Removes extraneous newlines. #22

merged 5 commits into from Sep 5, 2016

Conversation

ashfurrow
Copy link
Member

Fixes #20.

Regexes gave me trouble for stripping lines like

//: Some markdown.


I originally used squeeze("\n") but that strips consecutive newlines in code, which is not good (I added a test for that). So my solution was to split based on a regex that used \n* and then join. The problem is that this adds a newline before any matched components, so I make sure to remove newlines preceding //: comments after that check. Let me know what you think, I'm sure there's a better solution.

@ashfurrow
Copy link
Member Author

@rpowelll Are you available to look this over? No worries at all, I know you're busy!

# - Extraneous newlines before /*:
# - Extraneous newlines after */
# - Extraneous newlines either before or after //:
page_contents
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe break this out into a strip_extraneous_newlines method to make it more explicit, since this main process_page method will likely grow to have more responsibilities eventually.

@rhysforyou
Copy link
Contributor

rhysforyou commented Sep 5, 2016

It might be a good idea to add a test to ensure we're not stripping newlines from inside comment blocks as well since in Markdown those can be syntactically significant. My general rule is to over-test any time you use regular expressions since they tend to be fragile and introduce regressions.

@ashfurrow
Copy link
Member Author

That's a good idea!

@rhysforyou
Copy link
Contributor

👍 looks good

@rhysforyou rhysforyou merged commit d9a3d8f into master Sep 5, 2016
@ashfurrow ashfurrow deleted the flight branch September 5, 2016 23:31
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.

None yet

2 participants