idutils: Restructure module to be configurable and readable #104

sakshamarora1 · 2024-09-30T14:31:17Z

Closes: #103

ntarocco

The extension points are not yet implemented, right? Otherwise, can you point where?

idutils/__init__.py

idutils/utils.py

ntarocco

As discussed, we want to have a non-configurable core, and add extension points to implement your own IDs checks.
For example, keep PID_SCHEME as they are (the core ones), and also add a current_app.config["IDUTILS_CUSTOME_PID_SCHEMES"] to that we can implement our own. Similar way for the other config vars.

idutils/__init__.py

idutils/ext.py

sakshamarora1 · 2024-10-02T15:33:58Z

The config on the overlay would look like this:

IDUTILS_CUSTOM_PID_SCHEMES = [
    ("test", lambda x: True),
]

from invenio_rdm_records.config import RDM_RECORDS_IDENTIFIERS_SCHEMES, RDM_RECORDS_PERSONORG_SCHEMES
RDM_RECORDS_IDENTIFIERS_SCHEMES = {
    **RDM_RECORDS_IDENTIFIERS_SCHEMES,
    "test": {"label": _("Test"), "validator": lambda x: True, "datacite": "Test"},
}

RDM_RECORDS_PERSONORG_SCHEMES = {
    **RDM_RECORDS_PERSONORG_SCHEMES,
    "test": {"label": _("Test"), "validator": lambda x: True, "datacite": "Test"},
}

sakshamarora1 · 2024-10-02T15:35:40Z

Affected modules that would need a minor release:
(bottom to top)

cds-rdm
invenio-app-rdm
invenio-rdm-records
invenio-banners
invenio-communities
invenio-administration
invenio-drafts-resources
invenio-github
invenio-jobs
invenio-pages
invenio-requests
invenio-users-resources
invenio-vocabularies
invenio-records-resources
commonmeta-py
datacite
marshmallow-utils

idutils/config.py

tests/test_idutils.py

idutils/config.py

ntarocco · 2024-10-08T19:49:16Z

idutils/detectors.py

+    .. note:: Some schemes like PMID are very generic.
+    """
+    schemes = []
+    pid_schemes = PID_SCHEMES + current_app.config["IDUTILS_CUSTOM_PID_SCHEMES"]


Use entrypoints here?

ntarocco · 2024-10-08T19:49:44Z

idutils/ext.py

+    def init_idutils_registry(self):
+        """Initialize identifier registries."""
+        # For registering detect_identifier_schemes, etc. functions
+        self.identifier_detectors_registry = {}


This looks ok to me, but how is it used?

anikachurilova

Reviewed with @jrcastro2 , looks good to us 🚀
One question: is it possible to override it with the entrypoints, or only extend?

zzacharo

For the new entrypoints, is there a place in the module that we can document (apart from potentially the docs website) the available entrypoints? See here for example.

zzacharo · 2024-10-11T07:47:12Z

idutils/ext.py

+        """Initialize identifier registries."""
+        # For registering is_* checks (functions)
+        self.identifier_checks_registry = {}
+        self.register_entry_point(


minor: @sakshamarora1 was this a convention you followed? If not, what about:

Suggested change

self.register_entry_point(

self._load_entry_point(

zzacharo · 2024-10-11T07:50:59Z

idutils/utils.py

+
+import isbnlib
+
+doi_regexp = re.compile(


Can we maybe move the relevant regex patterns to a separate file e.g. regex.py? We can leave here actual util functions otherwise this will continue becoming a catch-all place....

Yes 👍
More options to consider: patterns.py or rules.py?

zzacharo · 2024-10-11T07:51:33Z

setup.cfg

+    idutils = idutils.ext:finalize_app
+invenio_base.api_finalize_app =
+    idutils = idutils.ext:api_finalize_app
+# Custom entrypoints for extending functionality


To be removed?

zzacharo · 2024-10-11T07:52:56Z

tests/conftest.py

+@pytest.fixture(scope="session")
+def create_app():
+    """Application factory fixture."""
+    return create_api


I think we miss some tests about the new entrypoint functionalities. Basically, we should test the different entrypoints registration and showcase examples of callable functions that are expected there.

zzacharo · 2024-10-11T08:25:19Z

idutils/ext.py

+        self.configs_registry = {}
+        self.register_entry_point(self.configs_registry, "idutils.configs")
+
+    def register_entry_point(self, registry, ep_name):


How does it look for the end user when they will want to use one of registered functions? I imagine something like identifier_checks_registry.get('my_function')? What about if we expose a property so the end user would do idutils.[checks, normalizers,configs].{my_function}? I am not sure we need to expose proxies to the registries...What do you think?

sakshamarora1 · 2024-10-15T08:03:30Z

idutils/proxies.py

+from flask import current_app
+from werkzeug.local import LocalProxy
+
+current_idutils = LocalProxy(lambda: current_app.extensions["idutils"])


I think renaming this to something that includes scheme and registry might be better.

I matched to the convention we are following e.g.: https://github.com/inveniosoftware/invenio-rdm-records/blob/master/invenio_rdm_records/proxies.py#L14

sakshamarora1 · 2024-10-15T08:05:42Z

idutils/utils.py

+
+import isbnlib
+
+doi_regexp = re.compile(


Yes 👍
More options to consider: patterns.py or rules.py?

* Adds an entrypoint to register new custom custom schemes * Ensures that the new schemes are not overriding existing ones * Adds invenio-app as test dependency required by pytest-invenio Co-authored-by: Saksham Arora <[email protected]>

zzacharo · 2024-10-15T09:52:14Z

#104 (comment)

That's good but maybe better to attack this when we work on the new major release?

kpsherva · 2024-10-15T11:32:12Z

idutils/validators.py

+"""Utility file containing ID validators."""
+
+
+from six.moves.urllib.parse import urlparse


are we still using six? should we keep it ?

tmorrell · 2024-10-15T15:46:19Z

I'm supportive of making the module extensible....but the current implementation breaks usage of idutils outside of flask #105

ntarocco reviewed Sep 30, 2024

View reviewed changes

idutils/__init__.py Show resolved Hide resolved

idutils/utils.py Show resolved Hide resolved

ntarocco reviewed Oct 2, 2024

View reviewed changes

idutils/__init__.py Show resolved Hide resolved

idutils/__init__.py Outdated Show resolved Hide resolved

idutils/ext.py Outdated Show resolved Hide resolved

kpsherva reviewed Oct 3, 2024

View reviewed changes

idutils/config.py Outdated Show resolved Hide resolved

fenekku reviewed Oct 3, 2024

View reviewed changes

tests/test_idutils.py Outdated Show resolved Hide resolved

kpsherva reviewed Oct 3, 2024

View reviewed changes

idutils/config.py Outdated Show resolved Hide resolved

sakshamarora1 force-pushed the feature/configurable_idutils branch from 4b0bbf9 to 954a171 Compare October 3, 2024 15:23

ntarocco reviewed Oct 4, 2024

View reviewed changes

idutils/config.py Outdated Show resolved Hide resolved

idutils/config.py Outdated Show resolved Hide resolved

idutils/config.py Outdated Show resolved Hide resolved

ntarocco reviewed Oct 8, 2024

View reviewed changes

anikachurilova approved these changes Oct 10, 2024

View reviewed changes

zzacharo reviewed Oct 11, 2024

View reviewed changes

zzacharo force-pushed the feature/configurable_idutils branch from ba8370f to ba76834 Compare October 14, 2024 10:40

sakshamarora1 commented Oct 15, 2024

View reviewed changes

zzacharo force-pushed the feature/configurable_idutils branch 3 times, most recently from 4dbe004 to 2a0efea Compare October 15, 2024 08:24

kpsherva reviewed Oct 15, 2024

View reviewed changes

release: v1.3.0

629ea6a

zzacharo force-pushed the feature/configurable_idutils branch from 2a0efea to 629ea6a Compare October 15, 2024 11:43

zzacharo merged commit aded7b9 into inveniosoftware:master Oct 15, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

idutils: Restructure module to be configurable and readable #104

idutils: Restructure module to be configurable and readable #104

sakshamarora1 commented Sep 30, 2024

ntarocco left a comment •

edited

Loading

ntarocco left a comment

sakshamarora1 commented Oct 2, 2024 •

edited

Loading

sakshamarora1 commented Oct 2, 2024

ntarocco Oct 8, 2024

ntarocco Oct 8, 2024

anikachurilova left a comment

zzacharo left a comment

zzacharo Oct 11, 2024

zzacharo Oct 11, 2024

sakshamarora1 Oct 15, 2024

zzacharo Oct 11, 2024

zzacharo Oct 11, 2024

zzacharo Oct 11, 2024

sakshamarora1 Oct 15, 2024

zzacharo Oct 15, 2024

sakshamarora1 Oct 15, 2024

zzacharo commented Oct 15, 2024

kpsherva Oct 15, 2024

tmorrell commented Oct 15, 2024

		"""Utility file containing ID validators."""


		from six.moves.urllib.parse import urlparse

idutils: Restructure module to be configurable and readable #104

idutils: Restructure module to be configurable and readable #104

Conversation

sakshamarora1 commented Sep 30, 2024

ntarocco left a comment • edited Loading

Choose a reason for hiding this comment

ntarocco left a comment

Choose a reason for hiding this comment

sakshamarora1 commented Oct 2, 2024 • edited Loading

sakshamarora1 commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anikachurilova left a comment

Choose a reason for hiding this comment

zzacharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzacharo commented Oct 15, 2024

Choose a reason for hiding this comment

tmorrell commented Oct 15, 2024

ntarocco left a comment •

edited

Loading

sakshamarora1 commented Oct 2, 2024 •

edited

Loading