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

Refactor python function to create commandline arguments #338

Closed
larsbergstrom opened this issue Apr 25, 2016 · 5 comments
Closed

Refactor python function to create commandline arguments #338

larsbergstrom opened this issue Apr 25, 2016 · 5 comments
Labels

Comments

@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Apr 25, 2016

In #331, we added a function to create the path string, make_cd_string:
https://github.com/servo/saltfs/blob/master/buildbot/master/files/config/factories.py#L118-L119

However, this could be cleaner! As seen in https://github.com/servo/saltfs/blob/master/buildbot/master/files/config/factories.py#L126-L127, there are more arguments in the array that are static.

In this fix, please rename the function and make it return an array with all of the arguments required, while still taking the same single argument (the command to execute).

@ashrwin
Copy link
Contributor

@ashrwin ashrwin commented Apr 27, 2016

Are you suggesting something like this ?

def make_cd_command(command):
    return ["bash", "-l", "-c", "cd /c/buildbot/slave/windows/build ; " + command ]

windows = ServoFactory([
    # TODO: convert this to use DynamicServoFactory
    # We need to run each command in a bash login shell, which breaks the
    # heuristics used by DynamicServoFactory.make_step
    steps.Compile(command= make_cd_command("./mach build -d -v"),
                  env=envs.build_windows),
    steps.Compile(command= make_cd_command("./mach test-unit"),
                  env=envs.build_windows),

    # TODO: run lockfile_changed.sh and manifest_changed.sh scripts
])
@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Apr 27, 2016

@ashrko619 Yes, that's the right direction. If you open a PR I have a few style nits, but it looks good otherwise.

@ashrwin
Copy link
Contributor

@ashrwin ashrwin commented Apr 27, 2016

@aneeshusa Thanks, I will open a PR, btw what are the style nits here ?

@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Apr 27, 2016

Here, we need to rename the make_cd_command function to reflect its updated scope, and there are some extra spaces.

Generally, I prefer to do reviewing on PRs because inline comments are clearer, so go ahead and open a PR and I'll make any future comments there.

bors-servo added a commit that referenced this issue Apr 27, 2016
Refactored python function to create command line arguments

This commit is for issue #338

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/342)
<!-- Reviewable:end -->
@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Apr 27, 2016

Fixed by #342.

@aneeshusa aneeshusa closed this Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.