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

Raise error if local_path or central_path does not point to project_name folder. #183

Merged
merged 6 commits into from
Oct 9, 2023

Conversation

JoeZiminski
Copy link
Member

@JoeZiminski JoeZiminski commented Sep 29, 2023

closes #180.

This PR enforces the local_path and central_path that the user inputs to the make_config_file to point to a folder with the same name as a the datashuttle project_name. This also required fixing a lot of tests which did not use this pattern.

Previously there was some discussion on the path that is passed to make_config_file. Currently it should be the folder path including the project folder name. This was also documented incorrectly.

The two would be to to fix the local_path and central_path to include the project-folder name, or fix the folder above and infer the project folder from the name of the initialised datashuttle project. After some back and forth (with myself) in this PR I went for the former for the below two reasons:

  1. Under the hood, the 'local_path' and 'central_path' should intuitively point to the project folder. Then searches are simply local_path / rawdata rather than local_path / project_name / "rawdata". The latter could be added as a getter-function but is an extra level of complexity / maintenance that provides no benefit. It provides no benefit because for an instantiated project, we never want to switch between project folders, and so the config local_path should always point to the specific project folder.

  2. It adds a level of robustness, in that it forces the user to input a path that explicitly matches the project_name they have input. We could search dynamically that the correct folder name exists in a root directory, but it seems easier to nip this in the bud at the offset and ensure everything is already aligned at the level of the config file.

Would be good to hear if this makes sense!

@JoeZiminski JoeZiminski force-pushed the make_local_path_config_input_more_robust branch 8 times, most recently from 22cdb28 to 0de6c00 Compare October 3, 2023 15:11
@JoeZiminski JoeZiminski changed the title Add check if passed local or central path does not end in the project… Raise error if local_path or central_path to not point to project_name folder. Oct 3, 2023
@JoeZiminski JoeZiminski force-pushed the make_local_path_config_input_more_robust branch from 0de6c00 to 6f473b5 Compare October 3, 2023 15:44
@JoeZiminski JoeZiminski changed the title Raise error if local_path or central_path to not point to project_name folder. Raise error if local_path or central_path does not point to project_name folder. Oct 4, 2023
@JoeZiminski JoeZiminski force-pushed the make_local_path_config_input_more_robust branch from 6f473b5 to 217cf9d Compare October 5, 2023 16:30
Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

Hey @JoeZiminski. I agree with the choice you've made here. I was convinced by your arguments for choosing this way over the alternative. The implementation seems to work.

I only left one minor comment concerning the example paths in the documentation.

Feel free to merge this.

docs/source/pages/documentation.md Outdated Show resolved Hide resolved
@JoeZiminski JoeZiminski force-pushed the make_local_path_config_input_more_robust branch from c539ba1 to 31bc584 Compare October 9, 2023 16:35
@JoeZiminski
Copy link
Member Author

Thanks a lot for the suggestions @niksirbi !

@JoeZiminski JoeZiminski merged commit 8d328cc into main Oct 9, 2023
17 checks passed
@JoeZiminski JoeZiminski deleted the make_local_path_config_input_more_robust branch October 9, 2023 17:33
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.

[BUG] Fix confusing treatment of paths / top level folder
2 participants