Fix handling of pathlib.Path objects

[jdavies-st]

This fixes handling of pathlib.Path objects when used for input files for Step.

It also removes the old, deprecated tmpdir pytest fixture and replaces it with the modern tmp_path equivalent, which provides a pathlib.Path instance.

Resolves #139

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.25%. Comparing base (8c8bdd2) to head (f9411a9).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   59.33%   59.25%   -0.09%     
==========================================
  Files          24       24              
  Lines        1606     1615       +9     
==========================================
+ Hits          953      957       +4     
- Misses        653      658       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braingram]

As updating build_config to also accept Path seems to match the efforts in this PR would you update that portion of Step to also accept a Path instance:

stpipe/src/stpipe/step.py

Lines 1376 to 1381 in ee5f1ee

	if "config_file" in kwargs:
	config_file = kwargs["config_file"]
	del kwargs["config_file"]
	config_from_file = config_parser.load_config_file(config_file)
	config_parser.merge_config(config, config_from_file)
	config_dir = os.path.dirname(config_file)

That should make this call to str unnecessary.

[jdavies-st]

This actually fails within ConfigObj unfortunately. It does not fully accept pathlib.Path objects. And since this is now a dependency instead of vendored in extern, not much I can do here. Though perhaps I could modify line 1379 to pass a str() of the path?

[braingram]

It looks like config_file is returned by this function and is documented to be a str. So perhaps converting config_file to a str on 1377?

[braingram]

Thanks for putting this together!

Do the changes in this PR allow us to use -p no:legacypath? (see asdf-format/asdf#1759 for details). If so I'd be all for adding that to pyprojec.toml to guard against future tests using tmpdir.

[jdavies-st]

Do the changes in this PR allow us to use -p no:legacypath? (see asdf-format/asdf#1759 for details). If so I'd be all for adding that to pyprojec.toml to guard against future tests using tmpdir.

Good question. I'll give it a try. I too was trying to think of a way to prevent the use of the tmpdir pytest fixture from being reintroduced in testing as well. 👍

[braingram]

Unfortunately it looks like the CI is currently using this config file for pytest runs of downstream (JWST and romancal). As those packages are still using tmpdir this is leading to failures. I would categorize fixing the CI to not use this config as "outside the scope of this PR" (but would not object to a fix for it even if it's included here). So for now my vote is for removing this option and opening an issue about the above config issue (which is preventing us from using no:legacypath).

[jdavies-st]

Sounds good. I'll remove this, and I'll open an issue here and in the downstream packages.

[braingram]

Thanks for all your work on this!

Regression tests running at:
https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/611/
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1231/

[braingram]

Roman regtests all passed
JWST regtests had 3 failures (all known and random failures for missing log messages that occur likely due to stpipe logger handling).