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

Remove mandatory argument for Reader/Engine #79

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

magnusuMET
Copy link
Collaborator

This is to make nesting of pyaro configurations easier. This is a breaking change and will affect pyaro-readers and pyaerocom

Copy link
Member

@heikoklein heikoklein left a comment

Choose a reason for hiding this comment

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

I don't understand why you want to remove the readers handler as mandatory argument? It makes it very clear that the reader only concerns the data-format, while the datasource must be in a different place.

You also change the handler (filename_or_obj_or_url) to a filename. Not all readers will implement a disk-base filename, but rather a url (ACTRIS/EBAS).

There is no issue related to this PR, so I don't see any why.

@magnusuMET
Copy link
Collaborator Author

There is a linked PR which should explain a bit more why I would like this change. It essence it allows serialising the arguments which the positional argument don't allow.

This is still a draft, I am working with @dulte to hash out some ideas (this being one of them)

@heikoklein
Copy link
Member

Looking at the PR in pyaro_readers, this looks to me like datasets is the handler her and just needs to be renamed to filename_or_obj_or_url?

@magnusuMET
Copy link
Collaborator Author

Not all datasets requires a filename_or_obj_or_url, such as the data-merger (or post-processor). The first argument of the reader is not required to be named filename_or_obj_or_url which means it has to be unpacked and given as a positional argument

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