Skip to content

improved sql comment handling#73

Merged
arthurnn merged 3 commits into
basecamp:masterfrom
travis-ci:igor-sql
Sep 21, 2018
Merged

improved sql comment handling#73
arthurnn merged 3 commits into
basecamp:masterfrom
travis-ci:igor-sql

Conversation

@igorwwwwwwwwwwwwwwwwwwww

Copy link
Copy Markdown
Contributor

This is an improved version of #56.

@igorwwwwwwwwwwwwwwwwwwww

igorwwwwwwwwwwwwwwwwwwww commented Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Please note that the tests are currently not passing when run on travis (https://travis-ci.org/travis-ci/marginalia/jobs/431465306). Possibly due to the dependency on the old mysql gem.

It also looks like the tests haven't been enabled for a while now though. So fixing that seems out of scope for this patch.

I was able to successfully run the tests locally via:

DRIVER=sqlite3 ruby -Ilib -Itest test/*_test.rb

Comment thread lib/marginalia/comment.rb Outdated
end

def self.escape_sql_comment(str)
str = str.dup

@arthurnn arthurnn Sep 21, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can't we only dup if the str includes the patterns bellow? I want to save the extra allocation if not needed.

@igorwwwwwwwwwwwwwwwwwwww

Copy link
Copy Markdown
Contributor Author

I've made the dup conditional. In fact I'm not sure if it is even needed. Since the gsub should not modify the original. So we may be able to drop the dup completely.

Comment thread lib/marginalia/comment.rb Outdated
def self.escape_sql_comment(str)
if str.include?('/*') || str.include?('*/')
str = str.dup
end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You are right, we don't need the dup. gsub should already create a new string for us.

@igorwwwwwwwwwwwwwwwwwwww

Copy link
Copy Markdown
Contributor Author

Removed the dup.

@arthurnn arthurnn merged commit 36d4d75 into basecamp:master Sep 21, 2018
@arthurnn

Copy link
Copy Markdown
Collaborator

Thanks. i am cleaning up test and releasing a new gem version

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.

2 participants