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 work for multi-line HTML #96

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

Conversation

SampsonCrowley
Copy link
Contributor

@SampsonCrowley SampsonCrowley commented Apr 30, 2019

fixes raw tag parsing mentioned in #95

@marcandre
Copy link
Collaborator

Interesting PR.

I only looked at it quickly and I'm not the final authority on this gem, but a few things come to mind:

  1. this mixes independent changes (config vs raw tag). Ideally these would be separate PRs. Also never change version numbers in a PR, that's the job of the maintainers.

  2. For the raw tag, the issue I see is that inky-rb is meant to be the Ruby port of inky, so it should probably support raw tags iff inky does.

  3. For the config part, I believe that ignoring bad values passed (e.g. test "will not set an invalid components override") is not the right way to go. Raise a TypeError instead.

@SampsonCrowley
Copy link
Contributor Author

SampsonCrowley commented May 3, 2019

@marcandre

  1. good point, I will split it into separate PRs

  2. this repo already supports raw tags, but the current implementation in this repo only checks a single line at a time. this is an HTML based library, that's not how users expect HTML parsing to work

  3. another good point, will fix

@SampsonCrowley SampsonCrowley force-pushed the develop branch 2 times, most recently from f479ec0 to 5c4e07e Compare May 3, 2019 01:05
@marcandre
Copy link
Collaborator

  1. Oh, right, my bad.

@SampsonCrowley SampsonCrowley changed the title add multiline raw parsing and fix components config fix raw tag parsing to work for multi-line HTML May 3, 2019
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