search

NASA-PDS / doi-service

Service and tools for generating DOIs for PDS bundles, collections, and data sets
https://nasa-pds.github.io/doi-service
Other
2 stars 3 forks source link

Issue #350 - dataCite reserve error on title #354

Closed alexdunnjpl closed 1 year ago

alexdunnjpl commented 1 year ago

🗒️ Summary

Resolves #350 (probably - given the fact that I was able to replicate @tloubrieu-jpl's experience with a fresh development environment and the fact that the error should have affected anyone using a venv and a user-defined config, I'm gonna guess this is the cause).

The path to the user-defined config at the project/repository root was incorrectly being resolved via sys.prefix (which is something like path/to/doi-service/venv when a venv is in use), and a logging bug was hiding the fact that the user-defined config was not being found/used.

This PR also fixes the logging bug, adds some explanatory context to the 404 response returned by DataCite, and improves the articulation of some comments and symbols.

⚙️ Test Data and/or Report

Unit tests pass (except flake8 lint)

♻️ Related Issues

closes #350

alexdunnjpl commented 1 year ago

@nutjob4life hey, I'm grateful for the conscientious review!

1/2 - PROJECT_SRC_DIR isn't used, it just seemed like a maybe-useful thing to include at the time. Given that it's actually the opposite of useful, for the reasons you've given, I think the best approach is to nuke it and avoid the issue entirely. The issue of not returning a correct value out of a wheel/egg is less of a (practical) problem for PROJECT_ROOT_DIR as it isn't a defined concept unless run from an editable installation. So it leaves the codebase less broken than it was, though that's kind of a low bar if there's a reasonable solution available.

Technically I guess the correct solution would be to have parse_project_root_dir() throw if it is run from a wheel/egg, have the codebase reference the function rather than defining a constant, and have any use catch the exception and behave appropriately (in this case, by not considering the project-dir-located config file as a candidate). Don't suppose you'd be able to save me the trouble of figuring out how to detect when it's being run from a wheel/egg vs otherwise? (if so I'll update, and re-open the PR, else throw it in the later basket)

3 - The (only) existing interface for user-specified logging I could see is the logging_level configuration value, so being new I figured I'd not rock the boat too much on that score. My thought is that anyone who needs to dig deeper than that is going to find it trivial to make whatever temporary changes they need in the logging code.

4 - I don't think you're wrong, but I'm too new to agree - I was deferring to the documentation, which states that the non-dev user-defined configuration can/should be provided in the repository/project root. os.getcwd() is not fit for (that) purpose, unless I'm misunderstanding. I do think that the current configuration approach is generally confusing, in terms of "use the default config from core/wherever but overwrite with values in $PROJECT_ROOT/pds_doi_service.ini if exists but overwrite with values in $some_other_dir_for_dev_cfg/pds_doi_service.ini if exists. That seems worth a new ticket in and of itself though.

Who are the core stakeholders/design-authorities on this repo besides @tloubrieu-jpl ? I'm not sure with whom I need to be talking to about design change suggestions where I'm not absolutely certain I'm not missing some context.

alexdunnjpl commented 1 year ago

Re-tested after removing PROJECT_SRC_DIR, unit tests pass.

jordanpadams commented 1 year ago

@alexdunnjpl you can discuss some of these changes with @tloubrieu-jpl , @jimmie , @viviant100 and myself. Kind of a team design effort here

nutjob4life commented 1 year ago

That seems worth a new ticket in and of itself though

This ↑

I'm still +1 to merge this PR but making finding configuration files following familiar patterns should be a separate goal.

nutjob4life commented 1 year ago

FYI, this might satisfy @sonatype-lift:

def get_logger(name: str | None = None, logging_level: int = logging.INFO):

EDIT: Woops, that's for Python 3.10.

For 3.9:

import typing
def get_logger(name: typing.Optional[str] = None, logging_level: int = logging.INFO):
alexdunnjpl commented 1 year ago

Well looks like I get to learn two nice little bits of sugar today (as opposed to Union[str, None]... tyvm!

Python 3.10 really seems to be bringing the goods.

Will open a new PR since I can't figure out how to restore/reattach the branch.

nutjob4life commented 1 year ago

My "other" project is on 3.10 and I am loving it 🥹