Python style fixes #484

gabrielkrell · 2017-10-29T04:18:13Z

Checklist

I have made a material change to the repo (functionality, testing, spelling, grammar)
I have read the [Contribution Guide] and my PR follows them.
I updated my branch with the master branch.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation about the functionality in the appropriate .md file
I have added in line documentation to the code I modified

Short description of what this PR does:

Cleans up Mail helper: reduces line count, uses Pythonic defaults
See commit notes for categories of fixes.

These things were just bugging me a bit. There's probably a "terser is better" or something in the Python Zen.

Note: I think existing tests should cover this since I didn't change any functionality, but I'll let Codecov yell at me if not. Tests pass with tox.

Initializers should use property setters like everyone else. The default value of None can be applied with the existing default parameters instead of the manually setting internal values to None.

A sensible default for a list property is an empty list. This change simplifies existing code because we no longer need to turn a None into [] when using an add() method for the first time, and checks in get()s are simpler.

- move import to top of file for consistency - simplify __init__ logic: make it more clear that there are two categories of input (old v. new-style), and take advantage of default Nones.

codecov-io · 2017-10-29T04:42:30Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1ab4285). Click here to learn what that means.
The diff coverage is 95.77%.

@@           Coverage Diff            @@
##             master    #484   +/-   ##
========================================
  Coverage          ?   85.1%           
========================================
  Files             ?      30           
  Lines             ?     920           
  Branches          ?     108           
========================================
  Hits              ?     783           
  Misses            ?      84           
  Partials          ?      53

Impacted Files	Coverage Δ
sendgrid/helpers/mail/bcc_settings.py	`90% <100%> (ø)`
sendgrid/helpers/mail/substitution.py	`94.11% <100%> (ø)`
sendgrid/helpers/mail/content.py	`89.47% <100%> (ø)`
sendgrid/helpers/mail/footer_settings.py	`88.46% <100%> (ø)`
sendgrid/helpers/mail/bypass_list_management.py	`91.66% <100%> (ø)`
sendgrid/helpers/mail/asm.py	`78.94% <100%> (ø)`
sendgrid/helpers/mail/click_tracking.py	`89.47% <100%> (ø)`
sendgrid/helpers/mail/sandbox_mode.py	`91.66% <100%> (ø)`
sendgrid/helpers/mail/spam_check.py	`88.46% <100%> (ø)`
sendgrid/helpers/mail/email.py	`97.29% <100%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ab4285...d94fdb6. Read the comment docs.

delirious-lettuce

@gabrielkrell ,

I'm curious as to what this PR really accomplishes. There seems to be a pattern in these classes:

assign a "private" value(s) to None (corresponds to a public property)
if a corresponding argument was not passed in to __init__, call the setter

This pattern is also what is shown in the Python docs on properties:

class Parrot:
    def __init__(self):
        self._voltage = 100000

    @property
    def voltage(self):
        """Get the current voltage."""
        return self._voltage

After briefly looking over your code, the main difference seems to be that all of the classes you modified now unnecessarily call the corresponding setter whether or not it is necessary. This was done by deleting the private variable (starting with _) and assigning to a public one instead (which has the same name as the @property decorated function).

For example, I took two classes, Ganalytics and SubscriptionTracking and added print statements after each setter.

GanalyticsNEW and SubscriptionTrackingNEW are your classes.
GanalyticsORIGINAL and SubscriptionTrackingORIGINAL are the original classes.

>>> g = GanalyticsNEW()
enable setter
utm_source setter
utm_medium setter
utm_term setter
utm_content setter
utm_campaign setter
>>> g2 = GanalyticsORIGINAL()
>>> s = SubscriptionTrackingNEW()
enable setter
text setter
html setter
substitution_tag setter
>>> s2 = SubscriptionTrackingORIGINAL()

Of course, this happens only when each class falls back on the default None arguments but, other than lines of code reduced, I'm curious as to how this is an improvement?

delirious-lettuce · 2017-10-31T18:44:54Z

sendgrid/helpers/mail/category.py

@@ -1,9 +1,7 @@
 class Category(object):

    def __init__(self, name=None):
-        self._name = None
-        if name is not None:
-            self._name = name


# Shouldn't this... self._name = name # be this? self.name = name

Will fix in next push.

@gabrielkrell ,

You deleted this code already as well...

delirious-lettuce · 2017-10-31T18:52:30Z

sendgrid/helpers/mail/asm.py

-        self._groups_to_display = None
-
-        if group_id is not None:
-            self._group_id = group_id


# Shouldn't this... self._group_id = group_id # be this? self.group_id = group_id

Thanks for the catch - will fix typo in next commit.

@gabrielkrell ,

I'm not sure how you are going to do this since you deleted this code already...

Oh shoot - I thought that my replacement code had the typo, not the original. Github's not too great on mobile but I'll poke at it soon.

Yeah so with this and the other changes I kept the typo - my new code should use the setter and not assign directly to the private variables. Oops!

delirious-lettuce · 2017-10-31T18:53:08Z

sendgrid/helpers/mail/asm.py

-            self._group_id = group_id
-
-        if groups_to_display is not None:
-            self._groups_to_display = groups_to_display


# Shouldn't this... self._groups_to_display = groups_to_display # be this? self.groups_to_display = groups_to_display

Will fix in next push.

@gabrielkrell ,

You deleted this code already too...

gabrielkrell · 2017-10-31T20:25:04Z

Hey @delirious-lettuce, thanks for the detailed code review. I'll push the changes you pointed out once I get a little time.

Calling the setters on __init__ is intentional behavior. Reducing line count is the primary goal, but I think it makes sense for the setters to handle all property changes, even those as simple as adding a None.

However, I hadn't seen the docs example you showed, and I found more examples elsewhere. Upon further looking I can't find any that show it "my way", but neither can I find anything that explains why direct access is used instead of the setter. (I also couldn't find a PEP that introduced properties, so my Google-fu needs work.) I'm happy to close this if it's not best practice, but I'm curious about why direct access is considered better here. I know there's overhead in calling setters, but performance doesn't seem to be a concern since we have stub getters and setters in places where direct access could be used. In my view it makes things simpler with a minor performance penalty.

delirious-lettuce · 2017-10-31T20:32:34Z

@gabrielkrell ,

I don't work for SendGrid or have any power over this PR, I was just reading through different PRs and became curious when I read yours. You are free to ignore my "review"!

That said, the main reason I became interested in your PR was that it was written differently than other code using @property that I was familiar with. I didn't know if that was good or bad so I just played around with it for a bit and that was the main difference that I saw (unnecessarily calling setters). I'm always looking to improve so I thought I would ask you in case there was something else I was missing that made it advantageous to write it that way.

gabrielkrell · 2017-10-31T21:17:48Z

@delirious-lettuce No, no worries! I appreciate the pair of eyes (plus Matt and Elmer are pretty overworked right now from Hacktoberfest). You're definitely right about standard practice, I suppose I'd never noticed. It still feels like the right thing to do to me, but I don't like going against convention. If someone else could elaborate a bit on why examples use direct access it'd be awesome - maybe we can let this sit until then?

IMO, we probably shouldn't be using properties at all, since there's no error checking or special behavior in most places and the whole point of properties is that you can put them in as necessary without changing the interface. Without them we'd have the benefits of both approaches. But that's kind of beside the point since it would be too much trouble to remove them, we'd lose docstrings, etc.

Here's how I'd sum up pros/cons:

Pros:

Less conditionals: reduce mental complexity, flat is better than nested, blah blah
Smaller line count: make codeclimate happier (marginal)
IMO it makes sense for the setters to handle all changes.
- Fails early: setter should always allow default value as an input

Cons:

More explicit (but this is an argument against setters in general too)
Unusual: python docs, standard implementations do it the old way.
- Why? More info useful.
Performance penalty from calling setters in __init__ (marginal)

Ultimately though if it's unexpected enough to make you do a double-take I think that overrides whatever subjective simplicity it brings. I'll leave it up for now to get more opinions but my guess is it'll get closed.

delirious-lettuce · 2017-10-31T21:38:53Z

@gabrielkrell ,

If someone else could elaborate a bit on why examples use direct access it'd be awesome - maybe we can let this sit until then?

Sure, no problem.

IMO, we probably shouldn't be using properties at all, since there's no error checking or special behavior in most places and the whole point of properties is that you can put them in as necessary without changing the interface. Without them we'd have the benefits of both approaches. But that's kind of beside the point since it would be too much trouble to remove them, we'd lose docstrings, etc.

Ya, you're right there isn't anything special in the getters/setters yet but I guess different functionality could be easily added in the future (if necessary). And yes, it also seems like a bit much to be removing them at this point to.

Performance penalty from calling setters in init (marginal)

I didn't want to make it seem like this was a big deal or anything (I didn't do any profiling to measure the difference), it was just that I couldn't find a lot of differences between the two different versions.

Ultimately though if it's unexpected enough to make you do a double-take I think that overrides whatever subjective simplicity it brings. I'll leave it up for now to get more opinions but my guess is it'll get closed.

I don't think I have enough experience to have that decision rest solely on my opinion of how it was written. As I've said, I was really just curious that this could be a better way to write it and was trying to figure out why. I'm still learning new things about Python on an almost daily basis and reading other developers code is one of the best ways that I've found to improve my own skills.

Thanks for your time!

gabrielkrell · 2017-10-31T21:57:30Z

Thanks @delirious-lettuce! I learned something new as well :)

mbernier · 2017-11-01T03:25:09Z

I am also of the mind that having setters and getters is ideal, because you can defer any future responsibility for modifying the parameters in the same place. this keeps the code simpler, when you're debugging - whereas having a direct access in one place with logic for how to set that param (defaulting, any ETL, or other modifications) and then also having a setter/getter that might have to do that seems redundant.

Having said all of that, it always "feels wrong" when making a basic class that is really just a clean way to organize and access parameters because it is so redundant.

I will defer to whatever the master, @thinkingserious, says here - my personal preference, verbose as it is, would be to make it a standard to have the getters and setters - which forces the issue for future code, as insurance against having parameters act one way on init and another on obj.param. This also has the impact of simplifying testing.

mbernier · 2017-11-01T03:26:42Z

I also feel the need to add - THANK YOU both @gabrielkrell and @delirious-lettuce for having a great conversation about this here. I was able to quickly join the conversation and throw my $.02 in, whether it was helpful or not is up for debate - but you saved me TONS of time and provided great information that we can refer to in the future. I really you both and your time spent here.

delirious-lettuce · 2017-11-01T03:27:00Z

@mbernier ,

Thanks for the input, that makes a lot of sense!

EDIT:

I've been having fun trying to do some reviews on these repos!

mbernier · 2017-11-01T03:29:06Z

@delirious-lettuce it is a tough call - on one hand, we are actually asking quite a bit - "Write all this seemingly redundant code to save what is likely 2 lines of duplication". In the future though, we can easily say "Add this logic to the getter on this object" and there is no confusion about what we are saying in the issue or where the logic should be.

For a project with, as of this month, hundreds of contributors - I can definitely see the value in being explicit.

delirious-lettuce · 2017-11-01T03:30:16Z

@mbernier ,

For a project with, as of this month, hundreds of contributors - I can definitely see the value in being explicit.

I would agree 100% (but my vote doesn't really matter haha)

Thanks to delirious-lettuce - these were in the original code but should have been caught here. *Probably* don't want to bypass the setters.

That wasn't too bad.

gabrielkrell · 2017-11-01T04:35:44Z

sendgrid/helpers/mail/tracking_settings.py

@@ -1,5 +1,6 @@
 class TrackingSettings(object):
    """Settings to track how recipients interact with your email."""
+


Accidental change in merge commit - added a newline between class docstring and __init__ in several places, but this is consistent with the rest of the code (and probably some PEP rule).

mbernier · 2017-11-02T15:53:11Z

@gabrielkrell

We have not been able to merge your Pull Request, but because you are awesome - we wanted to make sure you could still get a SendGrid Hacktoberfest shirt.

Please go fill out our swag form before Nov 5th and we will send the shirt! (We know that you might have tried this before and it didn’t work, sorry about that!)

You have till Nov 5th to fill out this form in order to get the Hacktoberfest shirt!

Thank you for contributing during Hacktoberfest! We hope to see you in the repos soon! Just so you know, we always give away a SendGrid shirt for your first ever non-Hacktoberfest PR that gets merged.

thinkingserious · 2017-11-11T16:30:29Z

Hello @gabrielkrell,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

mbernier · 2017-11-11T18:25:11Z

Hey @gabrielkrell - wanna chat as a contributor?

You can grab a time on my calendar

Will be great to catch up :)

gabrielkrell · 2017-11-19T21:56:03Z

@mbernier I'd love to - sorry for taking so long. I grabbed a slot Tuesday PM.

gabrielkrell added 3 commits October 28, 2017 21:56

Clean up unneeded None guards

25309a2

Initializers should use property setters like everyone else. The default value of None can be applied with the existing default parameters instead of the manually setting internal values to None.

Clean up None defaults for lists

9eeb37f

A sensible default for a list property is an empty list. This change simplifies existing code because we no longer need to turn a None into [] when using an add() method for the first time, and checks in get()s are simpler.

Email cleanup

caae889

- move import to top of file for consistency - simplify __init__ logic: make it more clear that there are two categories of input (old v. new-style), and take advantage of default Nones.

thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 29, 2017

thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest labels Oct 29, 2017

delirious-lettuce reviewed Oct 31, 2017

View reviewed changes

delirious-lettuce mentioned this pull request Oct 31, 2017

Fix master failure on travis (relating to ASM raise-assertion). #524

Closed

6 tasks

apigirl added hacktoberfest and removed hacktoberfest labels Oct 31, 2017

gabrielkrell added 2 commits October 31, 2017 23:02

Code review fixes

d1d0a80

Thanks to delirious-lettuce - these were in the original code but should have been caught here. *Probably* don't want to bypass the setters.

Merge remote-tracking branch 'upstream/master' into not-java

d94fdb6

That wasn't too bad.

gabrielkrell commented Nov 1, 2017

View reviewed changes

mbernier merged commit a403813 into sendgrid:master Nov 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python style fixes #484

Python style fixes #484

gabrielkrell commented Oct 29, 2017

codecov-io commented Oct 29, 2017 •

edited by codecov bot

Loading

delirious-lettuce left a comment •

edited

Loading

delirious-lettuce Oct 31, 2017

gabrielkrell Oct 31, 2017

delirious-lettuce Oct 31, 2017

delirious-lettuce Oct 31, 2017

gabrielkrell Oct 31, 2017

delirious-lettuce Oct 31, 2017

gabrielkrell Nov 1, 2017

gabrielkrell Nov 1, 2017

delirious-lettuce Oct 31, 2017

gabrielkrell Oct 31, 2017

delirious-lettuce Oct 31, 2017

gabrielkrell commented Oct 31, 2017 •

edited

Loading

delirious-lettuce commented Oct 31, 2017 •

edited

Loading

gabrielkrell commented Oct 31, 2017

delirious-lettuce commented Oct 31, 2017 •

edited

Loading

gabrielkrell commented Oct 31, 2017

mbernier commented Nov 1, 2017

mbernier commented Nov 1, 2017 •

edited

Loading

delirious-lettuce commented Nov 1, 2017 •

edited

Loading

mbernier commented Nov 1, 2017

delirious-lettuce commented Nov 1, 2017

gabrielkrell Nov 1, 2017

mbernier commented Nov 2, 2017

thinkingserious commented Nov 11, 2017

mbernier commented Nov 11, 2017

gabrielkrell commented Nov 19, 2017

		@@ -1,5 +1,6 @@
		class TrackingSettings(object):
		"""Settings to track how recipients interact with your email."""

Python style fixes #484

Python style fixes #484

Conversation

gabrielkrell commented Oct 29, 2017

Checklist

Short description of what this PR does:

codecov-io commented Oct 29, 2017 • edited by codecov bot Loading

Codecov Report

delirious-lettuce left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielkrell commented Oct 31, 2017 • edited Loading

delirious-lettuce commented Oct 31, 2017 • edited Loading

gabrielkrell commented Oct 31, 2017

Pros:

Cons:

delirious-lettuce commented Oct 31, 2017 • edited Loading

gabrielkrell commented Oct 31, 2017

mbernier commented Nov 1, 2017

mbernier commented Nov 1, 2017 • edited Loading

delirious-lettuce commented Nov 1, 2017 • edited Loading

mbernier commented Nov 1, 2017

delirious-lettuce commented Nov 1, 2017

Choose a reason for hiding this comment

mbernier commented Nov 2, 2017

You have till Nov 5th to fill out this form in order to get the Hacktoberfest shirt!

thinkingserious commented Nov 11, 2017

mbernier commented Nov 11, 2017

gabrielkrell commented Nov 19, 2017

codecov-io commented Oct 29, 2017 •

edited by codecov bot

Loading

delirious-lettuce left a comment •

edited

Loading

gabrielkrell commented Oct 31, 2017 •

edited

Loading

delirious-lettuce commented Oct 31, 2017 •

edited

Loading

delirious-lettuce commented Oct 31, 2017 •

edited

Loading

mbernier commented Nov 1, 2017 •

edited

Loading

delirious-lettuce commented Nov 1, 2017 •

edited

Loading