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

✨ NEW: Fish implementation. #9

Closed
wants to merge 1 commit into from
Closed

Conversation

GeigerJ2
Copy link
Collaborator

@GeigerJ2 GeigerJ2 commented Jun 4, 2023

Added ShellGenerator class to streamline different shell commands. Currently, it takes the shell_str as instance variable to select the appropriate code, while shell_str and env_file_path are populated in the .format() call when the shell commands are returned. This still seems a bit clunky, any ideas how to improve that?

Added from_env_file classmethod to ProjectConfig to populate config instance from .env file. Renamed set_key/get_key to write_key/read_key.

Instantiation of ProjectConfig passing default arguments of aiida_venv_dir and aiida_project_dir, as setting inside ProjectConfig did not seem to have intended effect?

Added `ShellGenerator` class to streamline different shell commands.
Currently, it takes the `shell_str` as instance variable to select the
appropriate code, while `shell_str` and `env_file_path` are populated in
the `.format()` call when the shell commands are returned. This still
seems a bit clunky, any ideas how to improve that?

Added `from_env_file` classmethod to `ProjectConfig` to populate config
instance from .env file. Renamed set_key/get_key to write_key/read_key.

Instantiation of `ProjectConfig` passing default arguments of
`aiida_venv_dir` and `aiida_project_dir`, as setting inside
ProjectConfig did not seem to have intended effect?
@mbercx
Copy link
Member

mbercx commented Dec 14, 2023

@GeigerJ2 thanks for the contribution, and sorry for only coming back to this now... Do you mind if I make some changes directly on top of this PR?

@mbercx
Copy link
Member

mbercx commented Dec 14, 2023

Was playing around with this and split up the PR in #15 and #16. Have a look and let me know what you think!

@GeigerJ2
Copy link
Collaborator Author

Took it for a spin at the version of PR #16 and everything seemed to work fine. I'm happy with the changes, so we can close this and approve the other ones?

@GeigerJ2 GeigerJ2 closed this Jan 16, 2024
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.

2 participants