Skip to content

Conversation

@tdruez
Copy link
Contributor

@tdruez tdruez commented Jul 16, 2021

… setting #237

Signed-off-by: Thomas Druez tdruez@nexb.com

… setting #237

Signed-off-by: Thomas Druez <tdruez@nexb.com>
@tdruez tdruez requested a review from pombredanne July 16, 2021 15:08
@tdruez
Copy link
Contributor Author

tdruez commented Jul 19, 2021

@pombredanne could you have a look at the changes before the merge?

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

See my comments inline.

scanpipe/apps.py Outdated
pipeline_classes = inspect.getmembers(module, is_pipeline)

if len(pipeline_classes) > 1:
raise ImproperlyConfigured("Only 1 Pipeline class allowed per file.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise ImproperlyConfigured("Only 1 Pipeline class allowed per file.")
raise ImproperlyConfigured(f"Only one Pipeline class allowed per pipeline file: {path}.")


elif pipeline_classes:
pipeline_class = pipeline_classes[0][1]
self.register_pipeline(name=module_name, cls=pipeline_class)
Copy link
Member

Choose a reason for hiding this comment

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

Are we checking that the name is new and unique and does not mask the existing pipelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tdruez and others added 4 commits July 19, 2021 20:55
Co-authored-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Thomas Druez <tdruez@nexb.com>
@tdruez tdruez merged commit 516949b into main Jul 20, 2021
@tdruez tdruez deleted the 237-pipelines-dirs branch July 20, 2021 05:53
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.

3 participants