Skip to content

Support both & and | for metasets - #69

Open
frostming wants to merge 5 commits into
add-tests-frostfrom
bugfix/67-metasets-or
Open

Support both & and | for metasets#69
frostming wants to merge 5 commits into
add-tests-frostfrom
bugfix/67-metasets-or

Conversation

@frostming

Copy link
Copy Markdown
Member

Fix #68

@frostming
frostming changed the base branch from master to add-tests-frost October 1, 2019 12:48
@frostming
frostming force-pushed the bugfix/67-metasets-or branch from d75f7ba to 118c776 Compare October 1, 2019 12:51
@codecov

codecov Bot commented Oct 1, 2019

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.82609% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.65%. Comparing base (4097493) to head (38472b2).

Files with missing lines Patch % Lines
src/passa/internals/markers.py 59.18% 13 Missing and 7 partials ⚠️
src/passa/internals/specifiers.py 76.92% 1 Missing and 2 partials ⚠️
src/passa/models/metadata.py 93.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           add-tests-frost      #69      +/-   ##
===================================================
+ Coverage            54.96%   57.65%   +2.69%     
===================================================
  Files                   42       42              
  Lines                 2700     2756      +56     
  Branches               475      495      +20     
===================================================
+ Hits                  1484     1589     +105     
+ Misses                1100     1027      -73     
- Partials               116      140      +24     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@frostming
frostming force-pushed the bugfix/67-metasets-or branch from 118c776 to 9f13589 Compare October 1, 2019 12:53
@frostming
frostming force-pushed the bugfix/67-metasets-or branch from 66e57d0 to 3166a7c Compare October 1, 2019 15:02
return str(self) == str(other)

def __lt__(self, other):
return hash(self) < hash(other)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I’m cool with &, |, and bool, but not sure about the others. == could work, but I’m hesitant since it can result in false negative. I don’t get why < is needed at all, and the implementation even less.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

== is used for deduplicating markers, false negatives(if there are any) won't do harm, while false positives will. < is a somewhat dirty hack to make sorting work, but I suspect we need sorting at all.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would suggest using a regular function with proper docstring instead (say a.is_identical_to(b)) since == has the potential to be misused in the future. I don’t think in-class sorting makes sense here since the logic is completely arbitrary; it’s better to pass in a custom key= argument when sorting instead.

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.

2 participants