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

1118 one from many msg extra checks #1134

Merged

Conversation

BibianaC
Copy link
Member

@BibianaC BibianaC commented Mar 4, 2019

Set up the ability to add one-from-many additional checks message (issue #1118).
The new messages are being prepared and will be added in a separate PR.

@BibianaC BibianaC requested review from Bjwebb and robredpath March 4, 2019 17:32
@Bjwebb
Copy link
Member

Bjwebb commented Mar 5, 2019

Note: I have checked it manually in my terminal. When I spin a local server I receive an error message: KeyError: 'validator about the last work you did @Bjwebb do I need to do anything locally to be able to run it?

You need the most recent commit of lib-cove. Installing from the requirements file should get this.

@@ -222,8 +224,11 @@ def produce_message(self):

def get_heading_count(self):
if self.grants_count == 1 and self.count == 1:
self.grants_percentage = int(100)
Copy link
Member

Choose a reason for hiding this comment

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

= 100 should be fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

return '1'
return '{:.0%} of'.format(self.count / self.grants_count)
heading_percentage = '{:.0%} of'.format(self.count / self.grants_count)
self.grants_percentage = int(heading_percentage[:-1])
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work, because heading_percentage contains the string of, so heading_percentage[:-1] is e.g. 14% o.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! I have fixed that.

Copy link
Member

@Bjwebb Bjwebb left a comment

Choose a reason for hiding this comment

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

The approach looks good to me.

@robredpath
Copy link
Member

I really like this approach:

  • We can have as many or as few bands for each check as we want
  • We can make the boundaries different for each check
  • We can easily port the existing checks and improve them as we go

@BibianaC
Copy link
Member Author

BibianaC commented Mar 5, 2019

@robredpath and I have agreed to add the ability to have one-from-many additional checks to all the checks. By doing this, this PR will be able to be merged and be ready when the new validation errors are prepared. (@robredpath @Bjwebb I will let you know when this is the case so you can review it)

@Bjwebb do you know why Flake8 is failing with errors that were not failing before?

@Bjwebb
Copy link
Member

Bjwebb commented Mar 6, 2019

@Bjwebb do you know why Flake8 is failing with errors that were not failing before?

When you added rangedict, you also updated the other requirements. Looks like the newest version of Flake8 picks up more errors than the older version.

@BibianaC
Copy link
Member Author

BibianaC commented Mar 7, 2019

@robredpath @Bjwebb The PR is more or less ready to review. The secret_data_tests is failing in assert json.load(fp) == json.load(fp_archive) when checking the difference between both files what I see is that the fp has an extra key validator. I don't know why this is happening.

@Bjwebb
Copy link
Member

Bjwebb commented Mar 8, 2019

Sorry, the failing secret_data_tests was introduced in this PR #1129 (comment).

requirements.txt Outdated Show resolved Hide resolved
@BibianaC
Copy link
Member Author

@Bjwebb If you think the code is good then it can be merged with the secret data tests failing.

@Bjwebb Bjwebb merged commit 795b435 into master-360-data-quality-tool Mar 13, 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.

3 participants