uperl / File-XDG

Basic implementation of the XDG base directory specification
1 stars 0 forks source link

`lookup_config_file` does not look at `config_home` #14

Closed van-de-bugger closed 2 years ago

van-de-bugger commented 2 years ago

lookup_config_file does look at config_home:

$ cat ./test.pl 
#!/usr/bin/perl
use strict;
use warnings;
use File::XDG;
printf( "File::XDG::VERSION=%s\n", $File::XDG::VERSION );
my $cfg = "test.conf";
my $xdg = File::XDG->new( name => 'Test' );
{
    printf( "config_home=%s\n", $xdg->config_home() );
    $xdg->config_home->mkpath();                # Make sure config_home exists.
    my $file = $xdg->config_home->file( $cfg );
    printf( "config_file=%s\n", $file );
    $file->spew( "XXX" );                       # Make sure config file exists.
    -f "$file" && printf( "$file exists.\n" );
}
my $file = $xdg->lookup_config_file( $cfg );    # Lookup for the config file.
printf( "config_file=%s\n", $file );

$ ./test.pl 
File::XDG::VERSION=0.08
config_home=/home/vdb/.config/Test
config_file=/home/vdb/.config/Test/test.conf
/home/vdb/.config/Test/test.conf exists.
Use of uninitialized value $file in printf at ./test.pl line 17.
config_file=

$ printenv | grep XDG_CONFIG

$ ls ~/.config/Test/
test.conf
plicease commented 2 years ago

I agree this is weird, but it is as documented:

https://metacpan.org/pod/File::XDG#lookup_config_file

Looks up the configuration file by searching for ./$subdir/$filename relative to all base directories indicated by $XDG_CONFIG_HOME and $XDG_CONFIG_DIRS. If an environment variable is either not set or empty, its default value as defined by the specification is used instead. Returns a Path::Class object.

lookup_config_file takes two arguments, the app-name and the filename so if you use $xdg->lookup_config_file( 'Test', $cfg ); you should get the the file that you intend. That said:

  1. The documentation could probably be more clear if it referred to the app-name or $name rather than $subdir.
  2. It might be reasonable to have a method that assumes the app-name provided by the constructor.
van-de-bugger commented 2 years ago

Ok, I see. Probably I didn't read the documentation carefully enough.

However, it is rather strange design decision: The File::XDG constructor requires app name, this app name is used by config_home, why it is not used by lookup_config_file? Why the app name is not used consistently?

plicease commented 2 years ago

Ok, I see. Probably I didn't read the documentation carefully enough.

However, it is rather strange design decision: The File::XDG constructor requires app name, this app name is used by config_home, why it is not used by lookup_config_file? Why the app name is not used consistently?

I agree! I'm not the original designer though just the maintainer, so I am not sure as to the why. I'd like to provide a more sensible interface, but would like to do it in a backward compatible way. One option I initially suggested would be to add methods to do things the more consistent way. Another option would be to provide a more consistent API if the object is constructed in a certain way (passing an api => 2 option perhaps).

use File::XDG;

my $xdg = File::XDG->new( name => 'Test', api => 2 );

# same as File::XDG->new( name => '...', api => 1 )->lookup_config_file( 'Test', 'test.cionf' );
my $path = $xdg->lookup_config_file( 'test.conf' ); 
plicease commented 2 years ago

I've started #17 to add experimental features for the version 1.00 API that would make these methods not take the app name, among other features. The new API will be opt-in so as to maintain backward compatibility.

plicease commented 2 years ago

I've released #17 as 0.09_01 which can be used to experiment with this new interface.

plicease commented 2 years ago

1.00_01 implements the final-ish form of this new/old API design.