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

Refactor everything to use correct canonical OS directories #96

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

kevinaboos
Copy link
Contributor

No description provided.

let preference_dir = project_dirs().preference_dir();
// TODO: this is overkill to create the directory every time.
// This should be done somewhere during init, but I'm not sure where.
std::fs::create_dir_all(&preference_dir).unwrap_or_else(|_|
Copy link
Collaborator

@jmbejar jmbejar Jun 7, 2024

Choose a reason for hiding this comment

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

The actual folder creation (if needed) is done in a previous call to setup_model_downloads_folder (see https://github.com/moxin-org/moxin/pull/96/files#diff-9cac44800b86084f5e0e0a00ab63cc15615598154b89126aea415e93019b7ca0R25)

So this line sounds like a duplication.. I'm pretty sure everything should work removing this line, but maybe you can confirm

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested it because I didn't understand why it was not failing after being invoked more than once. I've just convinced to myself that std::fs::create_dir_all is not erroring when the folder already exists (although it is hard to confirm from the docs!).

So, we can remove this line later, but no real harm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can't remove this line, but it should be moved elsewhere. Previously when everything was in the user's $HOME directory, a single call covered it. But in this case, the preferences directory may be in a completely different location than the app's data directory, so we need at least one call to fs::create_dir_all().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, without this line here, i get a runtime panic on my mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just convinced to myself that std::fs::create_dir_all is not erroring when the folder already exists (although it is hard to confirm from the docs!).

yes that is correct, that's the behavior of mkdir -p on Unix-like OSes.

@jmbejar jmbejar merged commit d79a375 into moxin-org:main Jun 7, 2024
@kevinaboos kevinaboos deleted the use_proper_project_directories branch August 19, 2024 20:03
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