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

initial implementation of run function to replace run_cmd + run_cmd_qa #4284

Merged
merged 10 commits into from Aug 7, 2023

Conversation

boegel
Copy link
Member

@boegel boegel commented Jun 22, 2023

cfr. #4252

Marked as draft because:

  • need to check which other tests can be copied to also cover run with the current implementation;
  • should switch from run_cmd to run where possible;

@boegel boegel added this to the 5.0 milestone Jun 22, 2023
@boegel
Copy link
Member Author

boegel commented Jun 23, 2023

@branfosj Up for taking a look at this and providing some feedback?

easybuild/tools/run.py Outdated Show resolved Hide resolved
Copy link
Member

@branfosj branfosj left a comment

Choose a reason for hiding this comment

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

I've gone through here and considered the discussion in #4252 and I'm happy with this appraoch.

Comment on lines +80 to +83
def run(cmd, fail_on_error=True, split_stderr=False, stdin=None,
hidden=False, in_dry_run=False, work_dir=None, shell=True,
output_file=False, stream_output=False, asynchronous=False,
qa_patterns=None, qa_wait_patterns=None):
Copy link
Member

Choose a reason for hiding this comment

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

A more general comment than specifically applying here, is do we want to move to using a specific standard for what we expect for arguements and docstrings? Something standard that we can then enforce using flake8.

I ask here, as I think I'd prefer these in alphabetical order.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with putting the options in alphabetical order, but the order used now isn't random: the most used options are basically listed first, and they're more-or-less grouped by type of option (like the qa_* ones).

Let's get some output from others on this (@lexming?), and not block the PR over this, since this is trivial to change in a follow-up PR if we care strongly enough.

Is there a way to selectively enforce alphabetical ordering for particular functions with flake8?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm inclined to keep this as is for now, as long as we make sure to make no assumptions about the order of named options when calling run, we can reorder things later here...

easybuild/tools/run.py Show resolved Hide resolved
@boegel
Copy link
Member Author

boegel commented Aug 2, 2023

Fix for broken tests is available in #4306

@easybuilders easybuilders deleted a comment from boegelbot Aug 3, 2023
@boegel boegel marked this pull request as ready for review August 3, 2023 17:44
@boegel boegel changed the title initial implementation of run function (WIP) initial implementation of run function Aug 3, 2023
@boegel boegel changed the title initial implementation of run function initial implementation of run function to replace run_cmd + run_cmd_qa Aug 3, 2023
Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

LGTM

Still a lot to go as regards implementation, but as long as the tests are there for each newly implemented option this should be fine.

test/framework/run.py Show resolved Hide resolved
Co-authored-by: ocaisa <alan.ocais@cecam.org>
@ocaisa ocaisa enabled auto-merge August 7, 2023 19:46
@ocaisa ocaisa merged commit 50457c3 into easybuilders:5.0.x Aug 7, 2023
32 checks passed
@boegel boegel deleted the run branch August 7, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants