Skip to content

Add support for Microsoft SQL Server#204

Merged
brianc merged 39 commits into
brianc:masterfrom
danrzeppa:master
Jan 19, 2015
Merged

Add support for Microsoft SQL Server#204
brianc merged 39 commits into
brianc:masterfrom
danrzeppa:master

Conversation

@danrzeppa
Copy link
Copy Markdown
Collaborator

It's been a while in the making, but this pull request has support for almost all of the functionality in node-sql. It took me a while to get around to getting it to a point where it is pushable. I've been using this code in some production systems for a while, however, so I'm confident that it works fairly well.

There have been some additions to the node-sql library, such as support for CASE which I have not implemented yet, but will be doing in the near future.

This is my first pull request, so forgive me if I've done something incorrectly. Since the branch I originally pulled is somewhat old, I'm not sure how well this will merge in. I'm not sure what the best way to handle it is. Perhaps I should merge all the current changes into my branch and then pull request on that?

danrzeppa and others added 30 commits August 31, 2013 12:31
… things and get the tests and everything working.
- Updated the test runner to run tests for "sqlserver"
- Updated select-tests and table-tests
…port COUNT(*) since SqlServer doesn't allow COUNT(tableName.*).
…s does not yet work because the syntax for SqlServer and the way visitXxx doesn't mesh well. Need to work on this. Also had to implement special code for renaming tables, adding columns, and dropping columns.
- Still needs to support schemas.
- SqlServer will throw an error on the arrayAgg function since it is not supported.
- Did some other misc cleanup.
@brianc
Copy link
Copy Markdown
Owner

brianc commented Jan 19, 2015

OH MY GOD THIS IS AMAZING! ⭐ 🌟 💫 👯

Comment thread lib/node/orderByValue.js Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

little bit of crazy indentation here

@brianc
Copy link
Copy Markdown
Owner

brianc commented Jan 19, 2015

Hey @danrzeppa - this is epic and awesome, truly. There are a few white space related issues I'm happy to clean up if you don't want to clean up. I'd only suggest you clean them up because then it will make your merge commit cleaner, but again, my editor will auto-format the entire file to 2 spaces no problemo. I don't want to nitpick on something so small as whitespace after so much awesome hard work has been done. Lemme know how you feel about that.

The best and what you get 3,000,000 bonus points for is all the test coverage! That's truly awesome. I'm very happy to give you contributor access to the repo if you'd like, hopefully to make it easier for to contribute and collaborate in the future. I'll also edit the README file to reflect it now has support for mssql!

3 cheers!

@danrzeppa
Copy link
Copy Markdown
Collaborator Author

@brianc Sorry about the spacing issues. Some of the other projects I work on use tabs, so usually I set my editor use tabs. I believe I have fixed all of them.

Contributor access would be cool. Initially I would still like another set of eyes on things, before I commit anything permanent; but I do think I have a pretty decent handle on things.

As I mentioned before I'm already looking to update the Microsoft SQL Server support to include the CASE functionality.

@brianc
Copy link
Copy Markdown
Owner

brianc commented Jan 19, 2015

Yah - we try to always work through pull requests. The only thing I commit directly to master are merge commits, doc changes, & npm version bumps, so you'll still get code reviewed. 😄

Pushing this epicness as a new minor version, adding you as contributor, and updating readme now.

brianc added a commit that referenced this pull request Jan 19, 2015
Add support for Microsoft SQL Server
@brianc brianc merged commit 20d4575 into brianc:master Jan 19, 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.

3 participants