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

Introduce applicationConfigHome #66

Merged
merged 2 commits into from Sep 17, 2021

Conversation

jonasfj
Copy link
Member

@jonasfj jonasfj commented Sep 15, 2021

We started moving dart pub configuration files here, see: dart-lang/pub#2999 (comment)

I figured this logic might live in cli_utils, then it's easy to use in dartdev, etc...

Should also make it easy to fix dart-lang/sdk#41560

Copy link
Member

@pq pq left a comment

Choose a reason for hiding this comment

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

Nice!

Just some nits...

CHANGELOG.md Outdated Show resolved Hide resolved
lib/cli_util.dart Outdated Show resolved Hide resolved
lib/cli_util.dart Show resolved Hide resolved
@jonasfj jonasfj merged commit c2dc871 into dart-lang:master Sep 17, 2021
@jonasfj jonasfj deleted the add-applicationConfigHome branch September 17, 2021 09:08
/// [XDG Base Directory Specification][1] on Linux and [File System Basics][2]
/// on Mac OS.
///
/// Throws if `%APPDATA%` or `$HOME` is undefined.
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
/// Throws if `%APPDATA%` or `$HOME` is undefined.
/// Throws if `%APPDATA%` or `$HOME` is needed but undefined.

/// * `$HOME/.config/<productName>` otherwise.
///
/// This aims follows best practices for each platform, honoring the
/// [XDG Base Directory Specification][1] on Linux and [File System Basics][2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have documentation for the windows convention?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried, I can't find any canonical source on MSDN... The closes I got was:
https://docs.microsoft.com/en-us/windows/deployment/usmt/usmt-recognized-environment-variables

But I have no idea what that page actually pertains to.. It looks like it says:

When using the XML files MigDocs.xml, MigApp.xml, and MigUser.xml, you can use environment variables to identify folders that may be different on different computers

So it pertains to people writing such XML files.. and not as general documentation of the Windows environment variables. Or at-least this is not clear to me 🙈

There is also references to Windows File System Namespace Usage Guidelines floating around, but again I can't find updated documentation from original source.

Who knows... maybe it's hidden behind an MSDN subscription? Or maybe it's just so widely known and there is so many search hits that finding the canonical documentation is very hard.

We could use wikipedia:

I just gave up 🤣


group('applicationConfigHome', () {
test('returns a string', () {
expect(applicationConfigHome('dart'), isA<String>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm - this is hard to test - but it seems we should be able to do better.

Could we at least confirm that the string ends with '${separator}dart'

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM - I wonder if we can write a better test though.

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

Successfully merging this pull request may close these issues.

Adhere to XDG base directory spec
3 participants