Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating EvaluatorBase #257

Merged
merged 15 commits into from
Nov 6, 2024
Merged

Creating EvaluatorBase #257

merged 15 commits into from
Nov 6, 2024

Conversation

ferag
Copy link
Collaborator

@ferag ferag commented Oct 16, 2024

No description provided.

@ferag ferag requested a review from orviz as a code owner October 16, 2024 12:55
api/rda.py Outdated
@@ -36,24 +36,22 @@ def wrapper(body, **kwargs):
# Get the identifiers through a search query
ids = [item_id]

# FIXME oai-pmh should be no different
downstream_logger = evaluator.logger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downstream_logger = None (evaluator is not set)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@orviz
Copy link
Collaborator

orviz commented Oct 17, 2024

Added changes to adapt EPOS plugin to EvaluatorBase

Copy link
Collaborator Author

@ferag ferag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where the list of config params should be loaded?

@@ -139,8 +139,8 @@ def __init__(self, item_id, oai_base=None, lang="en", config=None, name="epos"):
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config loading is already included in EvalutorBase. Do you think it is better to trust in super() class or we should keep this loading on each plugins?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it makes sense to use super(). . I'll make the change

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 00fa1c89dc0553d91cf468d486ac00f7c278ebed allows to pass (*args, **kwargs) from the Plugin class so you don't need to provide the list of input args to super(). In case you want to apply it to other plugins.

@orviz
Copy link
Collaborator

orviz commented Oct 18, 2024

@ferag EvaluatorBase has oai_base as input argument. Since we have plugins that don't use OAI-PMH, it would make sense to change the name of this variable.

@ferag
Copy link
Collaborator Author

ferag commented Oct 28, 2024

@ferag EvaluatorBase has oai_base as input argument. Since we have plugins that don't use OAI-PMH, it would make sense to change the name of this variable.

Do you think is it OK to use a general "api_endpoint" variable?

@orviz
Copy link
Collaborator

orviz commented Oct 29, 2024

@ferag EvaluatorBase has oai_base as input argument. Since we have plugins that don't use OAI-PMH, it would make sense to change the name of this variable.

Do you think is it OK to use a general "api_endpoint" variable?

sure. Last commit address this, but not tested all the plugins

@ferag ferag merged commit 80f0a8d into main Nov 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants