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

Refactor patterns; add tests, better blanks support #5

Conversation

eykd
Copy link
Contributor

@eykd eykd commented Apr 22, 2016

I apologize that this is all in one commit. I got going and the next thing I knew, I had gone too far without a commit.

Some highlights:

  • Compile patterns in poyo.patterns instead of _Parser.__init__. Compile flags are important contextual information that belong with the patterns themselves.
  • Add tests for compiled patterns.
  • Add better blank line support. The original impetus! Previously, blank lines in certain places, such as immediately after a section start or inside a list, would confuse the parser and raise an error. I've added support for blank lines in these situations.
  • Moved test YAML for parser test inside of test module. It wasn't really necessary to have since the parser only works with strings anyway, and it seemed easier to work on the test with everything in one place.

The blank line support probably isn't perfect yet, and the new LIST pattern has grown rather hideous, but the tests pass and I think it's a good start.

I apologize that this is all in one commit. I got going and the next thing I knew, I had gone too far without a commit.

Some highlights:

- **Compile patterns in `poyo.patterns` instead of `_Parser.__init__`.** This is important contextual information that belongs with the patterns themselves.
- **Add tests for compiled patterns.**
- **Add better blank line support.** The original impetus! Previously, blank lines in certain places, such as immediately after a section start or inside a list, would confuse the parser and raise an error. I've added support for blank lines in these situations.
- **Moved test YAML for parser test inside of test module.** It wasn't really necessary to have since the parser only works with strings anyway, and it seemed easier to work on the test with everything in one place.

The blank line support probably isn't perfect yet, and the new `LIST` pattern has grown rather hideous, but the tests pass and I think it's a good start.
@eykd eykd mentioned this pull request Apr 22, 2016
@eykd
Copy link
Contributor Author

eykd commented Apr 22, 2016

Closes #4. 😁

@hackebrot
Copy link
Owner

Thank you for this PR @eykd 🙇

Hope I'll get it to merge this one in in the next couple of days. There are a few changes I'd like to make to it though.

  • I agree that the compile flags belong to the patterns. For the sake of readability, I'd prefer to rename the string patterns rather than the compiled ones for better imports
  • Tests are awesome, I'll add some pytest touches to it. Using parametrized fixtures over loops will get us better test result reports
  • Didn't know blank lines in list values are a thing 😁 Not a big fan of YAMLs flexibility in that particular case, great to be able to support it though
  • I like to keep the YAML string in a separate file to run a local script to compare the parsed dicts of poyo against ruamel.yaml. Also I like syntax highlighting on the YAML text, which gets lost in Python code

@hackebrot hackebrot added the enhancement Enhancement proposal for poyo label Apr 26, 2016
@hackebrot hackebrot merged commit 87bf756 into hackebrot:master May 1, 2016
hackebrot added a commit that referenced this pull request May 1, 2016
@eykd eykd deleted the feature/refactor-to-add-tests-and-support-blank-lines branch May 2, 2016 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement proposal for poyo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants