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

Added a regex in the config file for each resource to exclude certain types of files #29

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

Conversation

JulienTD
Copy link
Contributor

@JulienTD JulienTD commented Feb 23, 2022

Description

Added a regex in the config file for each resource to exclude certain types of files.

Related Issue(s)

#28

@JulienTD JulienTD changed the title Added a filter Added a regex in the config file for each resource, to exclude certain types of files Feb 23, 2022
@JulienTD JulienTD changed the title Added a regex in the config file for each resource, to exclude certain types of files Added a regex in the config file for each resource to exclude certain types of files Feb 23, 2022
@JulienTD JulienTD force-pushed the file-filter branch 4 times, most recently from 1c64b16 to a8b9a76 Compare February 23, 2022 17:21
Copy link
Collaborator

@polyedre polyedre left a comment

Choose a reason for hiding this comment

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

I'm not really convinced by the addition of the enumeration. It adds a lot of complexity and I do not see the benefit of using it (We can debate this point if you think it's really useful.).

The PR also adds the optional exclude_regex argument to many methods. If we continue, we will have some methods with too much arguments in the future. In this case, I think that adding an attribute exclude_regex in the Ressource class could really simplify. You just have to use self.exclude_regex in method can_save.

Comment on lines +11 to +16
class ConfigField(Enum):
"""
This enum contains every field that can be found in the config file
"""
TARGETS = "targets"
EXCLUDE_REGEX = "exclude_regex"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je ne vois pas vraiment l'intérêt d'utiliser une énumération ici plutôt qu'une classe pure. Plutôt que de devoir passer l'argument config_field à chaque fonction, ça pourrait être plus simple pour la maintenabilité d'avoir un fichier definition.py contenant les abstractions des noms de champs. En pratique je suis pas certain qu'abstraire les champs soit utile puisqu'on aimerait assurer la stabilité de l'interface proposée par le json, et qu'il vaudrait mieux juste déprécier des champs si on souhaite faire des changements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je pense qu'une énumération est mieux pour la compréhension des fields disponibles dans le fichier de config parce qu'elle permet d'avoir un namespace: ConfigField et surtout un type pour récupérer le field que l'on souhaite. Pour parler de maintenabilité, on pourrait déplacer la classe ConfigField dans le fichier definition.py ce qui permettrait de tout regrouper au même endroit.

hashtheplanet/resources/resource.py Outdated Show resolved Hide resolved
tests/resources/test_resource.py Outdated Show resolved Hide resolved
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