Skip to content

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

Merged
merged 9 commits into from
Nov 12, 2020

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Nov 12, 2020

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.

cmaglie and others added 3 commits November 12, 2020 11:38
This prepares for building a compilation database later. The returned
command is not currently used anywhere yet, so this commit should not
change behaviour.
@@ -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)
Copy link
Contributor

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?

Copy link
Member Author

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 :-))

@cmaglie cmaglie merged commit df9f204 into arduino:master Nov 12, 2020
@cmaglie cmaglie deleted the legacy-removing-2 branch November 12, 2020 11:34
Copy link
Collaborator

@matthijskooijman matthijskooijman left a 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 :-)

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