Skip to content

Follow-up: Helpers for EPRE custom checks#14

Open
PiDelport wants to merge 5 commits intomasterfrom
followup-epre-custom-checks
Open

Follow-up: Helpers for EPRE custom checks#14
PiDelport wants to merge 5 commits intomasterfrom
followup-epre-custom-checks

Conversation

@PiDelport
Copy link
Copy Markdown

This follows PR #11: I ended up hacking on this at the Codebridge community evening last night. 🙂️

This:

  1. Tweaks the test assertions to check the full error content (to make sure we're actually reporting the right errors, and to have visibility of changes in output)
  2. Pulls out some generic code for cell, row, and column constraint checking

(This also applies isort and black to the touched code, in the hope of standardising in that direction.)

@PiDelport PiDelport added the enhancement New feature or request label May 3, 2019
@PiDelport PiDelport requested a review from jbothma May 3, 2019 13:53
@PiDelport
Copy link
Copy Markdown
Author

@delenamalan: I can't add you directly as a reviewer, but you may want to have a look at this too?

@deanmalan
Copy link
Copy Markdown
Contributor

Awesome! 💯 Don't you want to create a PR on the goodtables package itself as well with those Base* classes?

@PiDelport
Copy link
Copy Markdown
Author

@delenamalan: That's an option! My instinct is to let the code settle into use here a bit first, before submitting upstream, to let rough edges in the design / interface rough be worked out. (It's a lot harder to change interface once it's an upstream public API.)

PiDelport added 5 commits May 6, 2019 11:05
These implement common code for checking cell, row, and column
constraints.
Also format with black, while here.
These implement common code for checking cell, row, and column
constraints.
@PiDelport PiDelport force-pushed the followup-epre-custom-checks branch from a1fad68 to 5f5f661 Compare May 6, 2019 09:50
@PiDelport
Copy link
Copy Markdown
Author

(Rebased and updated.)

@deanmalan deanmalan self-requested a review May 6, 2019 11:30
@PiDelport
Copy link
Copy Markdown
Author

@delenamalan: Awesome, thanks. :)

@jbothma: Do you want to look over this too, or happy to merge?

@jbothma
Copy link
Copy Markdown
Contributor

jbothma commented May 21, 2019

Oh have you already brought it up to date with master? nice! I'll have a look ASAP. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants