Skip to content

build_cache.path correction #2921

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

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,11 @@ configuration file.

- on Linux (and other Unix-based OS) is: if
[`$XDG_CACHE_HOME`](https://specifications.freedesktop.org/basedir-spec/latest/#variables) is defined,
`$XDG_CACHE_HOME/arduino`. Otherwise `{HOME}/.config/arduino`.
- on Windows is: `{HOME}/AppData/Local/arduino`
- on MacOS is: `{HOME}/Library/Caches/arduino`
`$XDG_CACHE_HOME/arduino`. Otherwise `$HOME/.cache/arduino`.
- on Windows is: `%LocalAppData%/arduino`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- on Windows is: `%LocalAppData%/arduino`
- on Windows is: `%LOCALAPPDATA%/arduino`

This is the canonical capitalization:

> Get-Item -Path Env:LocalAppData

Name                           Value
----                           -----
LOCALAPPDATA                   C:\Users\per\AppData\Local

Copy link
Author

Choose a reason for hiding this comment

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

True, but Windows is case insensitive here.
It is more readable that just all upper or lower case though?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going down the path of discussing which of arbitrary forms of the variable name are most clear, I will argue that all caps most effectively communicates that we are referring to an environment variable, as that is the standard convention.

- on MacOS is: `$HOME/Library/Caches/arduino`

If neither `$HOME` nor `$XDG_CACHE_HOME` are defined, a temporary directory will be used.
Comment on lines +68 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If neither `$HOME` nor `$XDG_CACHE_HOME` are defined, a temporary directory will be used.

This is an emergency "belt and suspenders" fallback measure. We don't need to clutter up the documentation with information that will only be relevant for the <0.01% of users who have a completely borked operating system.

Copy link
Author

Choose a reason for hiding this comment

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

Seems more evil than reasonable by hiding vital implementation detail :)
The goal is not to please 0.01%, but to have an actual code implementation referenced as doc. In case implementation changes, I would urge to update the doc as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

My decision stands. If you don't remove this then the pull request will be rejected.

Copy link
Author

Choose a reason for hiding this comment

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

It is not belt and suspenders, if we are explaining in depth how the system works for the specific reason to avoid misunderstanding it.

Consider container with incorrectly configured env, or incorrectly sandboxed shell session
(...and how I got included in the 0.01% use-cases here...). Not truly impossible thing to happen.

Or a 2nd opinion, perhaps? I am truly misunderstanding the reasoning here, and the harsh response.


- The `directories.data` default is OS-dependent:

Expand Down