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

293 Remove default value for out_folder #319

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

HenrZu
Copy link
Contributor

@HenrZu HenrZu commented Jul 13, 2022

The problem here was that my path, in which the memilio folder was located, also contained the word memilio.
Previously, for the default value of `out_folder´, the absolute file path was splitted at the first appearance of 'memilio'.
The change makes it more flexible and allows higher subfolders to be named like "code_memilio" etc.

e.g. the default value for path .../code_memilio/memilio/.... would be .../code_/...

Closes #293
Closes #320
Closes #159

Merge Request - GuideLine Checklist

Guideline to check code before resolve WIP and approval, respectively.
As many checkboxes as possible should be ticked.

Checks by code author:

  • There is at least one issue associated with the pull request.
  • The branch follows the naming conventions as defined in the git workflow.
  • New code adheres with the coding guidelines
  • Tests for new functionality has been added
  • A local test was succesful
  • There is appropriate documentation of your work. (use doxygen style comments)
  • If new third party software is used, did you pay attention to its license? Please remember to add it to the wiki after successful merging.
  • If new mathematical methods or epidemiological terms are used, has the glossary been updated ? Did you provide further documentation ?
    is present or referenced. Please provide your references.
  • The following questions are addressed in the documentation*: Developers (what did you do?, how can it be maintained?), For users (how to use your work?), For admins (how to install and configure your work?)
  • For documentation: Please write or update the Readme in the current working directory!

Checks by code reviewer(s):

  • Is the code clean of development artifacts e.g., unnecessary comments, prints, ...
  • The ticket goals for each associated issue are reached or problems are clearly addressed (i.e., a new issue was introduced).
  • There are appropriate unit tests and they pass.
  • The git history is clean and linearized for the merge request.
  • Coverage report for new code is acceptable.

@HenrZu HenrZu marked this pull request as ready for review July 13, 2022 11:12
@HenrZu HenrZu requested a review from dabele July 13, 2022 11:12
@dabele
Copy link
Member

dabele commented Jul 13, 2022

Tiny improvement and I don't think it makes anything worse, so I'm inclined to approve it. But it doesn't solve the problem.

Scenarios where this still fails or produces unexpected results:

  • repository directory not named memilio
  • memilio-epidata package installed in python environment that is outside of the repository, e.g. global python environment
  • some parent folder of repository also called memilio

All of these are very reasonable, I have personally used all of them at some point. I still think the only proper way to do this is with no default value at all (or at most the current working directory)

@HenrZu HenrZu marked this pull request as draft July 13, 2022 13:34
@HenrZu HenrZu changed the title 293 Adjust splitting for default value of out_folder 293 Remove default value for out_folder Jul 18, 2022
@HenrZu HenrZu marked this pull request as ready for review July 18, 2022 06:43
@HenrZu
Copy link
Contributor Author

HenrZu commented Jul 18, 2022

I agree with you that directly removing the default value for the output folder is a better option. Originally I wanted to relate it to another issue and just fix my actual problem.
I have changed the code so that the value is no longer set by default
Thus we should be more independent of the naming in the file path and also of use in installed packages.

@xsaschako
Copy link
Member

@HenrZu What's needed here?

@mknaranja mknaranja mentioned this pull request Sep 25, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants