Skip to content

Resolved Issue #1915#1916

Merged
yawpitch merged 6 commits into
exercism:masterfrom
alirezaghey:issue-#1915
Sep 23, 2019
Merged

Resolved Issue #1915#1916
yawpitch merged 6 commits into
exercism:masterfrom
alirezaghey:issue-#1915

Conversation

@alirezaghey
Copy link
Copy Markdown
Contributor

@alirezaghey alirezaghey commented Sep 19, 2019

Currently, there are two minor issues with the generated test file which appear to be with the black auto formatter:
1- Dictionary entries are ALWAYS on separate lines, which makes them less easy to read when dealing with small entries. Like here
2- These very same entries have a redundant comma after their last entry. Like here

edit:
Resolved #1915

@alirezaghey alirezaghey requested a review from a team as a code owner September 19, 2019 06:35
Copy link
Copy Markdown
Contributor

@yawpitch yawpitch left a comment

Choose a reason for hiding this comment

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

1- Dictionary entries are ALWAYS on separate lines, which makes them less easy to read when dealing with small entries. Like here

I’d consider that more readable, not less, however readability of the tests isn’t as high a priority as consistency.

2- These very same entries have a redundant comma after their last entry. Like here

The advantage of the comma is that it allows a future commit to add a new entry line and not create a 2 line diff. It also allows entries to be arbitrarily rearranged / sorted without a potential SyntaxError.

I don’t think either of these are significant issues.

@alirezaghey
Copy link
Copy Markdown
Contributor Author

Great! Thank you for your swift review.
Two questions though:
1- Are the formattings part of the Black auto formatter?
2- Why's the CI check failing? Is this a problem with the checks in the generator file?

@yawpitch
Copy link
Copy Markdown
Contributor

Yes the formatting is black’s doing. We can expect it to make some changes relative to the hand-made templates. We could reduce that diff noise by going through and doing a PR that ran black on every non-auto-generated test file, but we’ve been handling it exercise by exercise as we introduce generator templates.

As for the failure that appears to be a bit of a race condition with #1914 and #1912 not being completed. I’ll try and clear that up today if possible.

Comment thread exercises/etl/.meta/template.j2 Outdated
Co-Authored-By: Corey McCandless <cmccandless@users.noreply.github.com>
@yawpitch yawpitch merged commit be83e63 into exercism:master Sep 23, 2019
@alirezaghey alirezaghey deleted the issue-#1915 branch September 23, 2019 18:35
dvermd pushed a commit to dvermd/python that referenced this pull request Oct 2, 2019
* added jinja2 template for etl auto test generation

* updated etl tests to v2.0.1 canonical data

* minor update to template

* Update exercises/etl/.meta/template.j2

Co-Authored-By: Corey McCandless <cmccandless@users.noreply.github.com>
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.

etl: update tests to v2.0.1

3 participants