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

fix raw tag parsing to include multi-line html #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SampsonCrowley
Copy link
Contributor

fixes raw tag parsing mentioned in #95

since this is an HTML based library, and raw tags are supported in the repo, only allowing single line parsing is a huge surprise. this fixes that surprise

lib/inky.rb Outdated Show resolved Hide resolved
@SampsonCrowley SampsonCrowley force-pushed the master-raw-tag-parser branch from 87a1ddc to f43db16 Compare May 3, 2019 17:00
@marcandre
Copy link
Collaborator

Cool. Last thing: wouldn't it be best (and simpler) to put those tests as new cases? I imagine that the output from the gem and the original js package were different for these and they simply should match.

@SampsonCrowley
Copy link
Contributor Author

@marcandre I'm not sure I follow, can you explain what you mean? As far as I remember (sorry not at my machine rn) I just added additional tests in the existing section that is testing this feature, to cover the expanded capability

@marcandre
Copy link
Collaborator

The cases_spec checks all examples in cases/ and compares the output of the gem with the original tool. For the the <raw> specs, we can either do like you did in this PR and check the actual output or, instead, do what I'm suggesting and compare with the output of the original tool. The later avoids a situation where the original tool doesn't allow multiline <raw> tags, for example but the gem does. The ultimate goal of the gem is to reproduce the behavior of the original tool.

@marcandre
Copy link
Collaborator

FWIW, I think the specs you edited predate the cases/

@SampsonCrowley
Copy link
Contributor Author

Sounds like I need to rebase and edit a lil bit

@SampsonCrowley
Copy link
Contributor Author

@marcandre so the main repo (I'm assuming at least) is also only parsing raw tags line-by line. does that mean I need to fix it there before proper multi-line HTML parsing will be allowed here for raw tags?

also after rebasing, it looks like the original <raw> tag testing never made it to the cases spec either FWIW

@SampsonCrowley
Copy link
Contributor Author

I will be fixing the multi-line issue in the main repo, so this fix can be properly moved to the cases spec without failing tests

@SampsonCrowley SampsonCrowley force-pushed the master-raw-tag-parser branch 2 times, most recently from 76df26b to 3a99db2 Compare May 6, 2019 02:41
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.

2 participants