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

DataDirs no longer used #54

Closed
gwillem opened this issue Apr 17, 2023 · 7 comments
Closed

DataDirs no longer used #54

gwillem opened this issue Apr 17, 2023 · 7 comments

Comments

@gwillem
Copy link

gwillem commented Apr 17, 2023

I was upgrading today from v0.0.0-20190319220657-88e5137d2444 to v0.4.0 and my app broke, because DataDirs is still present but doesn't seem to be used anymore (removed here: 1be1d4f#diff-5c0868fc869fb66c2b33ba837118f18c43f5002d0a3f8b9e11aceabe703d4b86L98)

I suggest dropping it altogether to prevent further confusion.

@adrg
Copy link
Owner

adrg commented Apr 18, 2023

Hi @gwillem

I suggest dropping it altogether to prevent further confusion.

Can you please explain the confusion part? The xdg.DataDirs (which is represented by the XDG_DATA_DIRS environment variable) contains additional paths which can be used by applications to store data files in. The main path relative to which applications should store data files is xdg.DataHome (represented by the XDG_DATA_HOME environment variable). Both are defined by the XDG spec so neither can be removed. Also, both xdg.DataHome and xdg.DataDirs are documented in the package.

xdg.DataDirs can be used by itself and the same paths are also used by the xdg.DataFile function as fallbacks for cases in which xdg.DataHome is not a writable location, for any reason.

I was upgrading today from v0.0.0-20190319220657-88e5137d2444 to v0.4.0 and my app broke, because DataDirs is still present but doesn't seem to be used anymore (removed here: 1be1d4f#diff-5c0868fc869fb66c2b33ba837118f18c43f5002d0a3f8b9e11aceabe703d4b86L98)

The commit you are referencing is before the first ever release/tag of the library. I don't do breaking changes between minor releases of the application unless I absolutely have to, in which case, I mention it in the release notes. However, in this case, you were not even using a released version.

Also, the commit you are referencing does not remove the use of xdg.DataDirs. The same paths are still used. See:

It is unfortunate that your app stopped working after the upgrade and while I don't know the exact reason for that, again, you were using an unreleased version of the library.

Also, you opened an issue which implies that I made a breaking change (which I did not) and your proposed solution is that I make a real breaking change, by removing xdg.DataDirs.

This is not a real issue and needs to be closed.

@gwillem
Copy link
Author

gwillem commented Apr 18, 2023

For the record, I am not accusing anyone, I am only grateful that you made this package, thanks for that!

while I don't know the exact reason for that

Commit 1be1d4f1c3bb00cccaa31a0a8044278c1c0d1cc6 changes from using DataDirs to using baseDirs.dataFile (which does not use DataDirs). So, AFAICT, DataDirs is no longer used internally, only exported.

func DataFile(name string) (string, error) {
-	return createPath(name, DataDirs)
+	return baseDirs.dataFile(relPath)
}

@gwillem
Copy link
Author

gwillem commented Apr 18, 2023

xdg.DataDirs can be used by itself and the same paths are also used by the xdg.DataFile function as fallbacks for cases in which xdg.DataHome is not a writable location, for any reason.

Maybe I misunderstand?

func Test_xdgDataDir(t *testing.T) {
	xdg.DataDirs = []string{"/tmp"}
	xdg.DataHome = "/nonexistent_location"
	path, err := xdg.DataFile("test.txt")
	assert.NoError(t, err)
	assert.Equal(t, "/tmp/test.txt", path)
	// /Users/willem/Library/Application Support/test.txt
}

@adrg
Copy link
Owner

adrg commented Apr 18, 2023

For the record, I am not accusing anyone, I am only grateful that you made this package, thanks for that!

while I don't know the exact reason for that

Commit 1be1d4f1c3bb00cccaa31a0a8044278c1c0d1cc6 changes from using DataDirs to using baseDirs.dataFile (which does not use DataDirs).

func DataFile(name string) (string, error) {
-	return createPath(name, DataDirs)
+	return baseDirs.dataFile(relPath)
}

That is what I'm saying:

This is what xdg.DataDirs was in the version you were using:

DataDirs = xdgPaths("DATA_DIRS", DataHome,
    []string{"/usr/local/share", "/usr/share"})

This is xdg.DataFile:

func DataFile(name string) (string, error) {
	return createPath(name, DataDirs)
}

In the "new" version, xdg.DataDirs is:

DataDirs = baseDirs.Data

which is:

baseDirs.Data = xdgPaths(envDataDirs, "/usr/local/share", "/usr/share")

This is xdg.DataFile:

func DataFile(relPath string) (string, error) {
	return baseDirs.dataFile(relPath)
}

which is:

func (bd baseDirectories) dataFile(relPath string) (string, error) {
	return createPath(relPath, append([]string{bd.DataHome}, bd.Data...))
}

In both versions, xdg.DataFile and xdg.SearchDataFile do the exact same thing.

@adrg
Copy link
Owner

adrg commented Apr 18, 2023

xdg.DataDirs can be used by itself and the same paths are also used by the xdg.DataFile function as fallbacks for cases in which xdg.DataHome is not a writable location, for any reason.

Maybe I misunderstand?

func Test_xdgDataDir(t *testing.T) {
	xdg.DataDirs = []string{"/tmp"}
	xdg.DataHome = "/nonexistent_location"
	path, err := xdg.DataFile("test.txt")
	assert.NoError(t, err)
	assert.Equal(t, "/tmp/test.txt", path)
	// /Users/willem/Library/Application Support/test.txt
}

Oh, I see now. Your application changes xdg.DataHome and/or xdg.DataDirs. They are not meant to be changed. They are "read-only" in that sense. Yes, in the version you are referencing, that would impact xdg.DataFile. In any case, they are not used internally in xdg.DataFile or xdg.SearchDataFile in any of the released versions of the package.

Why do you want to change xdg.DataHome/xdg.DataDirs in your application anyway? xdg.DataHome and xdg.DataDirs default to the values of the $XDG_DATA_HOME and $XDG_DATA_DIRS environment variables, which are usually set by the system and you are supposed to use those paths. If they are not defined, the library provides appropriate locations for them.

If you really want to change them, although you should not need to, you can do something like:

os.Setenv("XDG_DATA_HOME", "/nonexistent_location")
os.Setenv("XDG_DATA_DIRS", "/tmp")
xdg.Reload()

path, err := xdg.DataFile("test.txt")

@gwillem
Copy link
Author

gwillem commented Apr 18, 2023

Thanks for checking!

Why do you want to change xdg.DataHome/xdg.DataDirs in your application

To run unit tests.. Setting the OS envs works indeed, it just took me a while to figure that out :)

@gwillem gwillem closed this as completed Apr 18, 2023
@adrg
Copy link
Owner

adrg commented Apr 18, 2023

Thanks for checking!

Sure, no problem.

Why do you want to change xdg.DataHome/xdg.DataDirs in your application

To run unit tests.. Setting the OS envs works indeed, it just took me a while to figure that out :)

Ah, ok. You were changing them in unit tests. That's fair enough.

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

No branches or pull requests

2 participants