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

new: add yaml parser for projects config. #379

Closed
wants to merge 1 commit into from

Conversation

vaab
Copy link
Contributor

@vaab vaab commented Jan 30, 2015

Hum, I've just seen I'm not the first to provide this (see #359 )

I have no preference on which implementation is best.

@msabramo
Copy link
Contributor

msabramo commented Mar 5, 2015

Oops. Neither this nor #359 merges cleanly now.

@vaab vaab force-pushed the pr-yaml-parser branch 2 times, most recently from 82f0607 to faa09fd Compare March 12, 2015 10:23
@vaab
Copy link
Contributor Author

vaab commented Mar 12, 2015

Rebased the PR, added one more test... can you consider looking at this before another commit breaks it again ? (I have several other PR waiting for this one also). Or if you have concerns about my PR please voice them. Many thanks !

@hackebrot
Copy link
Member

Can you please rebase your branch on top of our master?
Your forks local master is 152 commits behind.

Thanks 🙇

@vaab
Copy link
Contributor Author

vaab commented Mar 27, 2015

Rebased

@msabramo
Copy link
Contributor

Cool

@vaab
Copy link
Contributor Author

vaab commented Mar 27, 2015

Please note that the test are failing due to an issue I've raised #434 and this is unrelated to my patch.

https://ci.appveyor.com/project/audreyr/cookiecutter/build/1.0.464/job/gd2y5dl06014rvua

@@ -68,3 +68,9 @@ class ContextDecodingException(CookiecutterException):
"""
Raised when a project's JSON context file can not be decoded.
"""


class ParsingError(SyntaxError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just raise SyntaxError instead of adding another custom exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we need to offer the possibility to make a distinction between standard SyntaxError (which could stem from any python code or other non-controlled places -- like libraries --) and what was identified as ParsingError from one of the parsers of this module (and that can not be generated by any other code as it's unique to this task) ?

The code already rely on this in this very patch to display an error message on what was identified as syntax error while parsing config file whatever the format of the config file. It's meant as a generalization of sub-parser's parsing exceptions (namely ScannerError for Yaml, and ... ValueError for json). Without this exception, I believe we are at risk of displaying a misleading error message to the user/dev, catching unrelated SyntaxError that could be casted in any context of any inner libraries.

Besides this rationale, it did seem rather natural for the project to not shy upon creation of custom exceptions as you can witness in the lines preceding the definition of this exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, convinced 👍

@shaleh
Copy link

shaleh commented Apr 14, 2016

What is holding this up? There was plenty of progress and then nothing......

@pydanny
Copy link
Member

pydanny commented Apr 14, 2016

What's holding this up is that we're all volunteers. No one gets paid to work on this project.

Also, we had to switch to Poyo (which involved @hackebrot creating it) for YAML parsing because PyYaml wasn't fully cross-platform (neither is ruamel.yaml). Some of us (or at least me) also think we should stick with the JSON format, or the HJSON (http://hjson.org/) alternative.

@shaleh
Copy link

shaleh commented Apr 15, 2016

This was intended as a gentle poke to see if anyone still cared. I get Open Source and volunteerism :-)

Thanks for the link to hjson. I had not seen that. Comments and the error from a dangling comma are the two big things I dislike about json.

@hackebrot
Copy link
Member

I struggle to read a "gentle poke" from "nothing......". 😒

We released a Cookiecutter 1.4.0 just recently, which comes with loads of improvements, such as support for jinja2 extensions and a lightweight yaml parser to overcome the numerous problems with known Python yaml parsers. We've also put a lot of effort in overhauling the test suite to not rely on network connection.

That being said, I don't think the file format is what's important here. There have been a bunch of requests and also efforts of the core team to move to a more advanced context file. Things like input validation, types, help messages, nested values.

If done right, it doesn't really matter if we go with json, yaml, toml, hjson or even ini files. All it takes is a rock solid parser which runs on Python 2.7, 3.3, 3.4, 3.5 and PyPy on Linux, OS X and Windows (json being builtin is an obvious solution in that regard) and a well-documented context format 😉

@pydanny
Copy link
Member

pydanny commented Jun 6, 2016

#743 moves us away from YAML. From there we can explore other formats for configuration.

@pydanny pydanny closed this Jun 6, 2016
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

6 participants