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

added storage dir path options #1426

Merged
merged 4 commits into from
Aug 17, 2023
Merged

added storage dir path options #1426

merged 4 commits into from
Aug 17, 2023

Conversation

Smug246
Copy link
Contributor

@Smug246 Smug246 commented Aug 15, 2023

From discussion #1418

@rodja
Copy link
Member

rodja commented Aug 16, 2023

I think we should not use the temp directory as default to store persistent data. temp gets potentially cleaned a lot, and would defeat the purpose of the persistence.

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks for the quick PR, @Smug246! Just a few remarks:

  • I have to agree with @rodja. At least in 1.3 we don't want to introduce breaking changes. So the default path should be the current working directory.
  • And I slightly tend to the parameter name storage_path instead of storage_dir to avoid an abbreviation and to indicate its path-like nature.
  • Last but not least we shouldn't forget to add this parameter to ui.run_with for when using NiceGUI with an existing FastAPI app.

@Smug246
Copy link
Contributor Author

Smug246 commented Aug 16, 2023

np ill make those changes

@falkoschindler
Copy link
Contributor

Thanks, @Smug246! I just made a few more changes:

  • I removed the special case of "temp". It doesn't make much sense to store data in the temp directory which might not survive a reboot. And it could be confused with a folder named "temp" in the current working directory. Therefore I removed it altogether. This doesn't prevent the user from explicitly setting the temp directory.
  • I converted the value to a Path object before storing it in globals. This way we can use it more easily.
  • I set the default value to Path('.nicegui') instead of None. This communicates the default value more clearly, removes the need for Optional and avoids the need for handling None later.

A downside of these changes is the duplication of the default value Path('.nicegui'). But since the parameter list of ui.run and ui.run_with is duplicated anyway, I guess this is a reasonable tradeoff.

@falkoschindler falkoschindler added this to the 1.3.10 milestone Aug 17, 2023
@falkoschindler falkoschindler added the enhancement New feature or request label Aug 17, 2023
@falkoschindler falkoschindler merged commit b10c031 into zauberzeug:main Aug 17, 2023
@falkoschindler
Copy link
Contributor

Oh dear, I merged too early. The storage path is already used before ui.run is called. Therefore the app can't start.
I'll somehow have to undo this PR on main...

@falkoschindler falkoschindler removed this from the 1.3.10 milestone Aug 17, 2023
falkoschindler added a commit that referenced this pull request Aug 17, 2023
@falkoschindler
Copy link
Contributor

This problem seems to be pretty fundamental. Therefore I reverted this PR and re-opened discussion #1418.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants