Kaggle / kaggle-api

Official Kaggle API
Apache License 2.0
6.01k stars 1.06k forks source link

Support XDG base directory specification on Linux #540

Closed Andriamanitra closed 1 month ago

Andriamanitra commented 5 months ago

This changes the default configuration directory on Linux to follow the XDG Base Directory Specification. To maintain backwards compatibility the ~/.kaggle directory is always used if it already exists.

Precedence for choosing the config directory:

  1. $KAGGLE_CONFIG_DIR if $KAGGLE_CONFIG_DIR is set and not empty
  2. ~/.kaggle if it exists
  3. (linux only) $XDG_CONFIG_HOME/kaggle if $XDG_CONFIG_HOME is set and not empty
  4. (linux only) ~/.config/kaggle
  5. ~/.kaggle

Some documentation will probably also need to be updated if this PR is to be merged, but I would first like to hear if this change/approach is welcomed.

closes #269

srstevenson commented 5 months ago

Should this be done on other Unix-like systems too, and not just Linux?

Andriamanitra commented 5 months ago

Should this be done on other Unix-like systems too, and not just Linux?

I'm unsure about BSD-based systems but MacOS has their own standard called Standard Directories. I don't know how widely it is adopted though.

Andriamanitra commented 2 months ago

This change is being made to generated code. The next time the package is released the code will be generated again and this change will be lost. Please make all changes to the source code, in the src directory.

I was little bit confused by this at first because the src/ directory didn't exist in the repository at the time the PR was opened, but the changes should hopefully be in the right place now.

stevemessick commented 2 months ago

We reorganized things a bit. This repo used to be a mirror of the primary, which was not public. This is now the primary source for Kaggle API. Previously, all the source code was in a dir named template, which was renamed to src. I'm not sure if the template dir was in this repo before, but I think it was.

Anyway, thanks for the update. I'll review it soon.

stevemessick commented 2 months ago

/gcbrun

stevemessick commented 2 months ago

This looks good, thanks! I have a few details, though. We're using a somewhat old yapf for formatting, and everything gets formatted during codegen. Could you change your code to use this format, please?

    config_dir = os.environ.get('KAGGLE_CONFIG_DIR')

    if not config_dir or config_dir == '':
        config_dir = os.path.join(expanduser('~'), '.kaggle')
        # Use ~/.kaggle if it already exists for backwards compatibility,
        # otherwise follow XDG base directory specification
        if sys.platform.startswith('linux') and not os.path.exists(config_dir):
            config_dir = os.path.join(
                (os.environ.get('XDG_CONFIG_HOME')
                 or os.path.join(expanduser('~'), '.config')), 'kaggle')

And, could you check that test/test_authenticate.py still works? I suspect it does not. To run it, you'll have to:

hatch run install
cd kaggle
python3 -m unittest test.test_authenticate

As you mentioned in your PR it would be good to update the instructions in docs/README.md, in the "API Credentials" section.

Andriamanitra commented 2 months ago
   if not config_dir or config_dir == '':

I'm not sure if the second condition was intentional addition, it wouldn't ever actually run when config_dir is '' because empty string is falsey. I could still add it for readability.

And, could you check that test/test_authenticate.py still works? I suspect it does not. To run it, you'll have to:

hatch run install
cd kaggle
python3 -m unittest test.test_authenticate

I fixed the test itself, however the installation script is using KAGGLE_DEV_CONFIG_DIR=$(realpath ~/.kaggle/dev) which creates ~/.kaggle. Should I change the dev config dir to follow XDG spec too?

stevemessick commented 2 months ago

Hmm, I wonder if that redundant or-clause was added by the experimental auto-programming tool I'm testing? Thanks for catching that.

And yes, updating the install script will probably make this change complete. Thanks!

On Fri, May 3, 2024 at 11:12 PM Mikko @.***> wrote:

if not config_dir or config_dir == '':

I'm not sure if the second condition was intentional addition, it wouldn't ever actually run when config_dir is '' because empty string is falsey. I could still add it for readability.

And, could you check that test/test_authenticate.py still works? I suspect it does not. To run it, you'll have to:

hatch run installcd kaggle python3 -m unittest test.test_authenticate

I fixed the test itself, however the installation script is using KAGGLE_DEV_CONFIG_DIR=$(realpath ~/.kaggle/dev) which creates ~/.kaggle. Should I change the dev config dir to follow XDG spec too?

— Reply to this email directly, view it on GitHub https://github.com/Kaggle/kaggle-api/pull/540#issuecomment-2094043053, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACA7VDKVCCBTD5Y6ULZFFC3ZAR34PAVCNFSM6AAAAABB6PPYI2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUGA2DGMBVGM . You are receiving this because you commented.Message ID: @.***>

stevemessick commented 1 month ago

/gcbrun

stevemessick commented 1 month ago

I'm not sure how this repo is configured. Do you need me to merge for you?

Andriamanitra commented 1 month ago

I'm not sure how this repo is configured. Do you need me to merge for you?

Yes, I don't have direct access to this repo. Thanks!

stevemessick commented 1 month ago

Done! Thanks again.