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

Switch to cson-safe for parsing #34

Closed
wants to merge 2 commits into from
Closed

Switch to cson-safe for parsing #34

wants to merge 2 commits into from

Conversation

jkrems
Copy link

@jkrems jkrems commented Mar 6, 2014

This uses the approach I outlined in a previous comment for parsing. I removed the test that requires parsing of invalid/unsafe coffee-script as part of a CSON file and moved test 8 to be now test 7. The new parser allows for statically evaluated arithmetic expressions (e.g. myFlag: 1 << 3). If that's a concern, I'd be happy to make it optional. I chose to implement the parsing in its own module since I think that having a minimalistic parsing library that just offers the JSON.{parse,stringify} interface could be valuable to people.

More information about the behavior of the parser:

@@ -48,7 +48,8 @@
"coffee-script": "~1.6.3",
"js2coffee": "~0.2.3",
"extract-opts": "~2.2.0",
"requirefresh": "~1.1.2"
"requirefresh": "~1.1.2",
"cson-safe": "~0.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

cson-safe is a fork of cson. This mean that when you download cson, you'll also be downloading a fork of cson. Couldn't we just add the functionality of cson-safe in as an option for cson?

Instead of adding package ontop of package, add functionality to the existing package.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why you linked to clyfe/cson.npm for cson-safe. That's not the repository of cson-safe. cson-safe lives under https://github.com/groupon/cson-safe/ and is not a fork of or shares any code with this repository. As for the explanation why I thought that creating a minimalistic parser is better: having a project that just focusses on the parsing, without added functionality like loading from files, increases reusability. For example it could make it possible to switch atom/season to the same parser without piling similar modules on top of each other.

Copy link

Choose a reason for hiding this comment

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

@RobLoach

We didn't want to replace cson or use any of it's code. I think you were just looking at the wrong repo.

Jan's goal was to implement a very slim, simple cson parsing engine that could be included in any other project that needed safe parsing and wanted to provide features on top of that. It's sort of like how request provides extra features on top of http to make it more usable for developers.

Maybe we should have called it safe-cson-parsing-core instead. That might have been a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it. I don't know where I got that link. It's this one, correct?
https://www.npmjs.org/package/cson-safe

Copy link
Author

Choose a reason for hiding this comment

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

Yep, that's the one. :)

@pflannery
Copy link

This looks good. I would like to see cson being json's equal too.
Looking at the code we could drop the require to coffee-script and also move coffee-script to a devDependency.

Although, one thing to look out for is projects like docpad where it uses cson to parse docpad.coffee which uses functions. This change would break those projects and they would need to change over use coffee-script parsing directly.

@balupton what say thee?

@jkrems
Copy link
Author

jkrems commented Mar 11, 2014

Moved coffee-script to devDependencies, did a smoke test with npm install --production and require('./'); in the repl.

@balupton
Copy link
Member

+1 for having safe cson in cson

The question is naming. As this repo is the most official cson, I wonder if it's worthwhile moving safe cson here and renaming this repo to cson-extras.

Otherwise, static cson is more appropriate (and dare I say more respectful) name than safe cson ;-)

@jkrems
Copy link
Author

jkrems commented Mar 16, 2014

In retrospect cson-parser would have been a better project name for cson-safe. If you feel strongly about it, I can change the name. Though that would mean also updating season and grunt-cson which already refer to the current name.

@jkrems
Copy link
Author

jkrems commented Mar 31, 2014

Just checking in to make sure I'm not currently the blocker on this. :)

@balupton
Copy link
Member

balupton commented Apr 4, 2014

Sorry, had some health issue recently. All better now. What is your npm username? I'll add you as a maintainer, as you'll be the best person to merge this in and release it :)

@jkrems
Copy link
Author

jkrems commented Apr 7, 2014

No worries, hope you are better now! :) Name on npm is also jkrems.

@jkrems
Copy link
Author

jkrems commented Dec 5, 2014

Rebased on latest master & updated cson-safe version to 1.0.0. Working on a PR to cson-safe to make stringify match more closely the current behavior.

{}
Copy link

Choose a reason for hiding this comment

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

Maybe add back the removed newline, to match the other test files?

Copy link
Author

Choose a reason for hiding this comment

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

The newline wasn't actually removed, this is what previously was 8.json.

Copy link

Choose a reason for hiding this comment

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

Ah, got it.

@balupton
Copy link
Member

balupton commented Feb 6, 2015

Fixed in v2

@balupton balupton closed this Feb 6, 2015
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