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

Escape backslash #32

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Escape backslash #32

wants to merge 7 commits into from

Conversation

fehays
Copy link
Contributor

@fehays fehays commented Feb 20, 2020

No description provided.

@jpmonette
Copy link
Owner

@fehays It seems like this PR's scope is much bigger than simply escaping backslash, no? Is it possible to break it down as I think the escaping backslash is a good addition, but there is probably room for discussing other changes.

Thanks

@fehays
Copy link
Contributor Author

fehays commented Apr 15, 2020

@jpmonette My mistake. I accidentally committed some other enhancements to this branch that I did not intend for this PR.

@fehays
Copy link
Contributor Author

fehays commented Apr 15, 2020

@jpmonette I've reverted the commits and published a new branch in my forked repo with the changes for supporting allowing parenthetical groupings of logical operators (https://github.com/fehays/q/tree/conditional-groups). Let me know if you want to discuss that enhancement

@jpmonette
Copy link
Owner

@fehays Do you mind just adding some simple tests to evaluate scenarios and then we should be good to go.

Thanks for the contribution!

@fehays
Copy link
Contributor Author

fehays commented Apr 15, 2020

@jpmonette Done. Thanks!

@jpmonette
Copy link
Owner

@fehays Quick update on my previous comment - if you could review.

@fehays
Copy link
Contributor Author

fehays commented May 1, 2020

@jpmonette Sorry, I may have missed a comment. What would you like me to review?

Copy link
Owner

@jpmonette jpmonette left a comment

Choose a reason for hiding this comment

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

@fehays Here!

.add(Q.condition('Name').equalsTo('Acme\\s Carwash'))
.build();

System.assertEquals('SELECT Id FROM Account WHERE Name = \'Acme\\\\\\\\s Carwash\'', query);
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure this is passing tests? Shouldn't it be \'Acme\\\\s Carwash\' instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpmonette Yes, it's passing. It is essentially \'Acme\\\\s Carwash\'. It looks strange because it's double escaped in the string literal for the expected output.
E.g.

System.debug('SELECT Id FROM Account WHERE Name = \'Acme\\\\\\\\s Carwash\'');
// output:
// SELECT Id FROM Account WHERE Name = 'Acme\\\\s Carwash'

@fehays fehays requested a review from jpmonette May 5, 2020 15:03
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