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

Improves data filtering reliability #2052

Merged
merged 8 commits into from Mar 10, 2022
Merged

Improves data filtering reliability #2052

merged 8 commits into from Mar 10, 2022

Conversation

mvilanova
Copy link
Contributor

No description provided.

@mvilanova mvilanova added the bug Something isn't working label Mar 9, 2022
@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert and fixes 4 when merging cf7d8fb into 92e854d - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

fixed alerts:

  • 2 for Module-level cyclic import
  • 1 for Non-iterable used in for loop
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request fixes 1 alert when merging 162c1ce into dfd5a2f - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@kevgliss
Copy link
Contributor

kevgliss commented Mar 9, 2022

At this point, how much of that library have we vendored in? I think at this point it might make sense to vendor in the rest and create it's own module (e.g. filtering) or some such.

@mvilanova
Copy link
Contributor Author

Roughly between 50-60% based on line count. We can look into vendoring the rest of the code in in a separate PR.

@mvilanova mvilanova marked this pull request as ready for review March 10, 2022 00:29
@mvilanova mvilanova requested a review from kevgliss March 10, 2022 00:30
@@ -304,7 +304,7 @@ def get_incident_forecast(
if i == 1:
incidents = create_incident_metric_query(
db_session=db_session,
filter_spec=common['filter_spec'],
filter_spec=common["filter_spec"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevgliss you may want to double check that your black plugin is running correctly.

self.function = function
self.filters = filters

def get_named_models(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This func was refactored to use SortedSet()

return cls


def get_named_models(filters):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This func was refactored to use []

@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2022

This pull request fixes 1 alert when merging a91d85e into dfd5a2f - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@mvilanova mvilanova merged commit 72897ae into master Mar 10, 2022
@mvilanova mvilanova deleted the bugfix/filtering branch March 10, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants