-
-
Notifications
You must be signed in to change notification settings - Fork 404
Some small refactoring on legacy package #1064
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
Conversation
This prepares for building a compilation database later. The returned command is not currently used anywhere yet, so this commit should not change behaviour.
740d7d0
to
706d825
Compare
@@ -189,22 +148,11 @@ func TrimSpace(value string) string { | |||
} | |||
|
|||
func PrepareCommand(pattern string) (*exec.Cmd, error) { | |||
parts, err := ParseCommandLine(pattern) | |||
parts, err := properties.SplitQuotedString(pattern, `"'`, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this change the behaviour of the compile
command --build-property
and --build-properties
flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it should work exactly the same. This is code duplication that has never been removed.
(BTW a quick test would not be bad :-))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit late, but I had a look over this PR and the changes look good to me. I'll also look at part 2 next :-)
These are smaller/medium changes that are preparatory for other bigger PR.
I found myself juggling those small patches here and there in other branches, so I've decided to bite the bullet and put them in a smaller PR and merge them once for all.
Each commit has an self-contained change, they should be fairly easy to review.
I've another batch of changes that sits on top of this PR.