Skip to content

Escape column names on add/change/drop#54

Merged
ssh24 merged 1 commit intomasterfrom
escape-columnName
Jul 12, 2017
Merged

Escape column names on add/change/drop#54
ssh24 merged 1 commit intomasterfrom
escape-columnName

Conversation

@ssh24
Copy link
Copy Markdown
Contributor

@ssh24 ssh24 commented Jul 12, 2017

Add escape function on the column names in add, change and drop operations to allow the underlying database deal with the name escaping.

connect to #53

@ssh24 ssh24 self-assigned this Jul 12, 2017
@ssh24 ssh24 requested a review from qpresley July 12, 2017 15:12
Copy link
Copy Markdown
Collaborator

@qpresley qpresley left a comment

Choose a reason for hiding this comment

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

I think line 80 escapes the name incorrectly. It should be using the columnEscape call as each underlying database may require different escape'ing.

@ssh24 ssh24 force-pushed the escape-columnName branch from 4e46394 to 038a5cf Compare July 12, 2017 15:19
@ssh24
Copy link
Copy Markdown
Contributor Author

ssh24 commented Jul 12, 2017

@qpresley Removed L80 and added the column escape function on the property name.

@ssh24 ssh24 force-pushed the escape-columnName branch 2 times, most recently from 4e4d242 to a8ec2c7 Compare July 12, 2017 18:22
@ssh24 ssh24 force-pushed the escape-columnName branch from a8ec2c7 to 8c3ebf9 Compare July 12, 2017 18:29
Copy link
Copy Markdown
Contributor

@loay loay left a comment

Choose a reason for hiding this comment

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

LGTM

@ssh24
Copy link
Copy Markdown
Contributor Author

ssh24 commented Jul 12, 2017

@slnode test please

@ssh24 ssh24 merged commit 1441859 into master Jul 12, 2017
@ssh24 ssh24 deleted the escape-columnName branch July 12, 2017 21:04
@ssh24 ssh24 added this to the Sprint 40 - Apex milestone Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants