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

feat(api): add app-specific directory APIs, closes #5263 #5272

Merged
merged 13 commits into from Sep 28, 2022

Conversation

caesar
Copy link
Contributor

@caesar caesar commented Sep 23, 2022

I'd appreciate some feedback on the approach taken here as it's my first time working with the Tauri codebase.

In particular, should there be tests? I can't find tests anywhere for similar API features such as the existing appDir method.

Also, are you happy with the way I have refactored appDir and logDir to internally call appConfigDir and appLogDir to avoid duplicated code?

I also wondered if there's an equivalent to @since for the Rust documentation comments?

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

closes #5263

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

In particular, should there be tests? I can't find tests anywhere for similar API features such as the existing appDir method.

It is fine, these functions don't have tests.

Also, are you happy with the way I have refactored appDir and logDir to internally call appConfigDir and appLogDir to avoid duplicated code?

Yeah that's perfect.

I also wondered if there's an equivalent to @since for the Rust documentation comments?

I don't think there is but it is not needed. We are using @since in the js docs because we on

tooling/api/src/path.ts Outdated Show resolved Hide resolved
tooling/api/src/path.ts Outdated Show resolved Hide resolved
core/tauri/src/api/path.rs Outdated Show resolved Hide resolved
core/tauri/src/api/path.rs Outdated Show resolved Hide resolved
core/tauri/src/app.rs Outdated Show resolved Hide resolved
tooling/api/src/fs.ts Show resolved Hide resolved
@FabianLars
Copy link
Sponsor Member

Another thing we need to think about is localdata vs data because of windows :/

@amrbashir
Copy link
Member

yeah we could also add appLocalDataDir

@caesar
Copy link
Contributor Author

caesar commented Sep 23, 2022

Thanks for the review @amrbashir, I'll make those changes later. I was actually going to ask about deprecation notices too but forgot, so thanks for mentioning that.

I'll look at appLocalDataDir too.

@caesar caesar marked this pull request as ready for review September 23, 2022 23:32
@caesar caesar requested a review from a team as a code owner September 23, 2022 23:32
@caesar
Copy link
Contributor Author

caesar commented Sep 23, 2022

I think I've addressed all the issues discussed, and also fixed the lint error. Would you mind taking another look @amrbashir ?

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Also missing change file, should be a minor bump

core/tauri/src/api/path.rs Show resolved Hide resolved
.changes/app-dirs-api.md Outdated Show resolved Hide resolved
Co-authored-by: Amr Bashir <amr.bashir2015@gmail.com>
amrbashir
amrbashir previously approved these changes Sep 25, 2022
lucasfernog
lucasfernog previously approved these changes Sep 28, 2022
@lucasfernog lucasfernog merged commit 5d89905 into tauri-apps:dev Sep 28, 2022
@caesar caesar deleted the api/dirs branch October 13, 2022 08:45
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.

[feat] Introduce a appDataDir path
4 participants