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

Optional facts #88

Merged
merged 5 commits into from
May 12, 2020
Merged

Optional facts #88

merged 5 commits into from
May 12, 2020

Conversation

srueg
Copy link
Contributor

@srueg srueg commented May 12, 2020

Change the CI setup to skip docker for tox and use GitHub actions matrix build.

Write tests to ensure behaviour of target rendering.

Signed-off-by: Simon Rüegg <simon@rueggs.ch>
@srueg srueg requested a review from simu May 12, 2020 08:21
@srueg srueg force-pushed the optional-facts branch 6 times, most recently from 88276b1 to fa8ca92 Compare May 12, 2020 09:02
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

LGTM overall, but it would be great if you can move the variable renaming from commit 0de17c8 into a separate commit to avoid bundling unrelated stuff in a single commit. Having unrelated changes in the same commit usually leads to unnecessary confusion when trying to figure out why a change was made.

commodore/git.py Show resolved Hide resolved
srueg added 4 commits May 12, 2020 11:49
The Docker setup is very slow since it copies around a lot of files.

Signed-off-by: Simon Rüegg <simon@rueggs.ch>
To also make the region optional.

Signed-off-by: Simon Rüegg <simon@rueggs.ch>
To make Flake8 happy

Signed-off-by: Simon Rüegg <simon@rueggs.ch>
@srueg srueg force-pushed the optional-facts branch from fa8ca92 to edb978d Compare May 12, 2020 09:51
@simu simu merged commit 189dc96 into master May 12, 2020
@simu simu deleted the optional-facts branch May 12, 2020 11:16
@srueg srueg mentioned this pull request May 12, 2020
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