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

Keep same extension when created tmp config file #885

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bouthilx
Copy link
Member

The users script may not work properly if the config file does not have the expected extension. We should keep the same one instead of turning it into .conf.

@hnekoeiq

The users script may not work properly if the config file does not have
the expected extension. We should keep the same one instead of turning
it into .conf.
@bouthilx bouthilx added bug Indicates an unexpected problem or unintended behavior low Superficial problem that doesn't affect the correctness (e.g., formatting, spelling, colors) labels Apr 13, 2022
@bouthilx bouthilx requested a review from lebrice May 9, 2022 21:08

assert original_ext == tmp_ext

shutil.rmtree(trial.working_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it necessary to do this? Shouldn't the working directory in the config always be based on the temp_path fixture?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -197,9 +194,16 @@ def get_execution_environment(self, trial, results_file="results.log"):

return env

def _consume(self, trial, workdirname):
def _prepare_config(self, trial, workdirname):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I know your stance is to add type hints to new modules, but I think new methods might also be a good opportunity to add type hints.

@bouthilx
Copy link
Member Author

Must adapt doc after merging #957

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior low Superficial problem that doesn't affect the correctness (e.g., formatting, spelling, colors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants