Skip to content

Conversation

@dpassen
Copy link
Collaborator

@dpassen dpassen commented May 26, 2020

@coveralls
Copy link

coveralls commented May 26, 2020

Pull Request Test Coverage Report for Build 207

  • 4 of 4 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 205: 0.0%
Covered Lines: 82
Relevant Lines: 82

💛 - Coveralls

Copy link
Collaborator

@cbefus cbefus left a comment

Choose a reason for hiding this comment

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

Overall it seems like a pretty minimal change and will force continued consistency going forward. I am on board. Some discussion about formatting before we merge. This is also just a "for the devs" change so I wont need a version/deploy.

value = optional.if_present(some_thing_consumer).or_else(or_else_procedure)
assert scope['if_seen']
assert not scope['else_seen']
value = optional.if_present(some_thing_consumer).or_else(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume we did a line break here because of the new 80 character limit? My preference would be:

value = \
    optional.if_present(some_thing_consumer) \
        .or_else(or_else_procedure)

This formatting ensures that the readers eye always sees the indented level as directly (or as closely as possible) subbed to the item it relates.

Off this explanation I also get people on my team doing things like:

value = \
    optional \
        .if_present(some_thing_consumer) \
        .or_else(or_else_procedure)

I dont mind this, as I think it reads really easy, but something about that lone optional \ is a bit strange.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if black can be configured like this. It presents itself as an 'opinionated' formatter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

¯_(ツ)_/¯

I could pull and try and figure it out but I am pretty impartial on linters. I can live with the above formatting if its not configurable. Curious about your preferences on the above styles though.

optional = Optional.empty()
with pytest.raises(TestException):
optional.if_present(lambda x: x).or_else_raise(TestException("Something"))
optional.if_present(lambda x: x).or_else_raise(
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar comment to above about formatting.


optional.if_present(some_thing_consumer).or_else_raise(ShouldNotHappenException)
assert scope['seen']
optional.if_present(some_thing_consumer).or_else_raise(
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above.



CompatibleABC = ABCMeta('ABC', (object,), {'__slots__': ()})
CompatibleABC = ABCMeta("ABC", (object,), {"__slots__": ()})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that it prefers double quotes to single, this has always been my preference. For some reason I feel it has been historically anti-pythonic though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can tell black to skip string normalization and it will leave single quoted strings as they are. It prefers doubles as text is more likely to include an apostrophe or single quote than a double quote.

@dpassen dpassen closed this Nov 28, 2021
@dpassen dpassen deleted the black-formatter branch November 28, 2021 00:29
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.

4 participants