Refactor

[ofek]

Changes:

1. Put conditional platform logic at the module level rather than within each function, speeding up calls and making the code much easier to maintain
2. Removed 2 erroneous calls to os.path.expanduser on macOS
3. Converted temporary dict creations on some Windows logic to if branches
4. Converted os.getenv calls with defaults to checking existence in os.environ

[Julian]

I can close my eyes and make believe I don't know anything (or maybe Bernat has an opinion) but to me aren't you a bit uncomfortable without some tests already being in place in appdirs for that kind of refactor? The extent of the tests I saw were basically "assert functions return strings" which is obviously quite rudimentary.

Wouldn't things feel a bit more comfortable if we had platform-specific tests (that were skipped on other platforms) but that asserted against the real expected directories via the GHA CI? And which then we could be sure had the same values after this refactor?

And yeah tell me to be quiet (or that we should do that later) :) if you just think we should merge this immediately. After all, scanning it, yeah, I'd expect this not to change behavior.

[ofek]

As a compromise, can the first version of tests merely assert that every API matches what appdirs returns?

[Julian]

Definitely happy with that compromise here!

[Julian]

(What you're noticing now by the way is that I didn't make CI use the tox.ini, it just uses running the test runner as before -- if you want to just hack in installing appdirs into the workflow file I think that's fine and I'll probably be happy to do it, but of course if you're happy to make that change yourself that's also nice :)

[ofek]

Passing. Also made #2

[Julian]

Hooray!

[ofek]

@gaborbernat Does this look alright to merge?

[gaborbernat]

I'd prefer to put the calls behind a cache layer. I don't want to run the manifestation on import, would prefer to be run on request but cached 🤔

[ofek]

What would you like cached?

[gaborbernat]

Ah miss-read the code.