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

Badgeo #238

Merged
merged 6 commits into from
Dec 18, 2019
Merged

Badgeo #238

merged 6 commits into from
Dec 18, 2019

Conversation

Maarten-vd-Sande
Copy link
Member

What problem is the PR solving / What's new?
As described in #228

What did change
Checking for layout now happens both on the website as through the API. If the website is unambiguous then use website info, otherwise API. Did not seem to break anything so far on my part,

@Maarten-vd-Sande
Copy link
Member Author

p.s. GEO is complaining, ill rerun tests later

Copy link
Member

@siebrenf siebrenf left a comment

Choose a reason for hiding this comment

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

What is worse/better:
-adding all imports in the configuartion.smk, making a single list, but adding imports that are not always needed
-adding imports per rule, keeping the list clear, but having two dozen lists everywhere

Right now we use both systems partially. At least we should pick one and stick to it. My preference is to place all imports in configuration.smk (and maybe add a comment indicating in which rules it is used).

@Maarten-vd-Sande
Copy link
Member Author

For me the most sensible is to put the imports above the file it is used. Including duplicate imports. This is the way it is done in 'normal' Python, and I think we should try to do things as Pythonic as possible since everything is based on it.

@Maarten-vd-Sande Maarten-vd-Sande merged commit 27628af into develop Dec 18, 2019
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