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

Issue 1431: fix copy multiple locators + small refactoring #1439

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

MariiaNebesnova
Copy link
Contributor

No description provided.

@MariiaNebesnova MariiaNebesnova changed the title Issue 1431: fix copy multiple locators Issue 1431: fix copy multiple locators + small refactoring Aug 2, 2023
Comment on lines +5 to +14
switch (match) {
case "\\":
return "\\\\\\\\";
case "'":
return "\\'";
case "\n":
return "\\n";
default:
return match;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like magic strings to me now.
Is it possible for you to add an explanation or constants with meaningful names to the code explaining them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, not my code. but it's covered with tests, hope they could help you to understand what's going on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just refactored location of this function

Copy link
Contributor

Choose a reason for hiding this comment

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

what tests cover this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MariiaNebesnova MariiaNebesnova merged commit 137a4c1 into master Aug 3, 2023
2 checks passed
@MariiaNebesnova MariiaNebesnova deleted the issue_1431 branch August 3, 2023 07:36
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