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

Add sql-parser.js #779

Merged
merged 2 commits into from
May 29, 2014
Merged

Add sql-parser.js #779

merged 2 commits into from
May 29, 2014

Conversation

megawac
Copy link

@megawac megawac commented May 9, 2014

Handy little library :) I didn't minify it as it's compiled via coffee-script anyway; uglify only saves a few bytes (15% smaller)

@megawac
Copy link
Author

megawac commented May 9, 2014

btw I modified sql-parser.js slightly so the amd define is define("sql-parser", ...) instead of define(...) which I think would be a good practice to encourage in the future with other projects. Otherwise it makes /g/ unusable for amd projects.

@jimaek
Copy link
Member

jimaek commented May 10, 2014

@pnommensen Should we merge? I am not a js dev so not 100% sure what is going on.

@pnommensen
Copy link
Contributor

I like the modification. However, perhaps it's better for @megawac to fork https://github.com/forward/sql-parser and maintain a version with the modification(s)?

In my opinion it's not a great practice to modify people's code and still say it's their creation. I personally wouldn't like that if it were my project -- even if the modification(s) make sense.

@megawac
Copy link
Author

megawac commented May 10, 2014

As discussed over irc, I think it makes sense to allow modifications to change anonymous amd declarations as it doesn't make sense for the case of a cdn and doesn't change the usage for the end user.

Quoting the underscore source

AMD registration happens at the end for compatibility with AMD loaders that may not enforce next-turn semantics on modules. Even though general practice for AMD registration is to be anonymous, underscore registers as a named module because, like jQuery, it is a base library that is popular enough to be bundled in a third party lib, but not be part of an AMD load request. Those cases could generate an error when an anonymous define() is called outside of a loader request.

@andykent do you have any objections?

jimaek added a commit that referenced this pull request May 29, 2014
@jimaek jimaek merged commit e117f17 into jsdelivr:master May 29, 2014
@megawac megawac deleted the patch-1 branch May 31, 2014 19:06
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

3 participants