Use platformdirs for path locations

[blink1073]

Fixes #234

[blink1073]

I don't think we can make this change, I'm running into ActiveState/appdirs#167.

  File "C:\hostedtoolcache\windows\Python\3.10.7\x64\lib\site-packages\appdirs.py", line 134, in site_data_dir

    path = os.path.normpath(_get_win_folder("CSIDL_COMMON_APPDATA"))

  File "C:\hostedtoolcache\windows\Python\3.10.7\x64\lib\site-packages\appdirs.py", line 481, in _get_win_folder_with_pywin32

    dir = shell.SHGetFolderPath(0, getattr(shellcon, csidl_name), 0, 0)

pywintypes.com_error: (-2147024893, 'The system cannot find the path specified.', None, None)

[blink1073]

Looks like platformdirs is a drop-in replacement and is maintained.

[jasongrout]

Looks like platformdirs is a drop-in replacement and is maintained.

And maintained by Bernat, at that. 👍

[jasongrout]

How should we manage the transition? I can imagine this will be a big pain to people that have configs in our hardcoded places. At the very least, it seems like we should have a deprecation period for the old directories. Probably even better is to have an opt-in to the new behavior on this next major version (with deprecation of old directories), and an opt-out in the major version after that, then final removal of old directories in the major version after that?

[blink1073]

Probably even better is to have an opt-in to the new behavior on this next major version (with deprecation of old directories), and an opt-out in the major version after that, then final removal of old directories in the major version after that?

Yeah, that sounds reasonable.

[jasongrout]

To elaborate more, here's a proposal: we have a JUPYTER_PLATFORM_DIRS environment variable (or a better name?)? If JUPYTER_PLATFORM_DIRS is truthy, we apply this new logic, and if it is falsey we apply the old logic.

• In version 5, if JUPYTER_PLATFORM_DIRS is not defined, we assume it is falsey. We also introduce a deprecation warning when using the old logic. (However, there is a question about how we might make sure users get the deprecation warning. We might use logic similar to what we introduced in ipywidgets to bubble the deprecation from Calculate correct stack level for deprecation warnings to show them to the user jupyter-widgets/ipywidgets#3569)
• In version 6, we flip the default so that if JUPYTER_PLATFORM_DIRS is not defined, we assume it is truthy
• In version 7, we remove the environment variable checks and old logic.

[jasongrout]

@blink1073 - I pushed a commit consolidating the environment checking to a single function that also uses our envset utility function.

[jasongrout]

@blink1073 - this all looks good to me now. If it looks good to you, I think we can merge. Thanks!

[blink1073]

Great! I'll cut a 5.0 rc tomorrow

[dlqqq]

This change is causing CI failures in my branches. Is the deprecation warning intended to raise an exception?

Build log: https://github.com/jupyter-server/jupyter_server_fileid/actions/runs/3222840389/jobs/5272324591

[dlqqq]

I was able to resolve the failures by explicitly setting JUPYTER_PLATFORM_DIRS=1 in my CI environment. jupyter-server/jupyter_server_fileid@941ab67

[kevin-bates]

Hi @dlqqq - your pyproject.toml treats warnings encountered in pytest as errors.

Using jupyter-server as an example, this particular DeprecationWarning can be tolerated by adding it to the filters.
(At least that's my understanding of things.)

[dlqqq]

@kevin-bates Ah, you're right. So, warnings.warn() only fails in Pytest and not at runtime? Seems very odd, and I can't find this behavior documented anywhere.

[kevin-bates]

Yeah, I had to dig a bit, but found this and remember @blink1073 mentioning something about taking a stronger stance regarding warnings in one of his updates.

If you remove the error entry from the filterwarnings entry, pytest will succeed as well (and display the deprecation warning following the tests).

[blink1073]

Yep, that's right, see for example how jupyter_server handles warnings in tests.