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

[New Concept]: Decorators #2838

Merged
merged 49 commits into from
Jun 16, 2022
Merged

[New Concept]: Decorators #2838

merged 49 commits into from
Jun 16, 2022

Conversation

mathstrains21
Copy link
Contributor

@mathstrains21 mathstrains21 commented Jan 3, 2022

Adds concept / concept exercise for #2356. This is in a WIP state and is up for any feedback to come in earlier!

Notes:

  • error messages have been designed in line with python 3.9. I believe the error messages in question have not been changed between 3.6 and 3.9. I am aware that 3.10 changed many error messages, but I do not know whether the ones used in this concept have been changed. If you would like me to check/update these to be inline with 3.10 then please say! (I chose 3.9 as it is the version currently used by the test runner and I believe that 3.8, which the track itself uses, doesn't make a difference anyway.

In line with the above, should I be basing code samples off 3.8 or 3.9? (I assume when the track updates code samples would have to be updated to)

I will edit this as I add things that are useful to note and when I mark this PR as ready for review!

@github-actions

This comment was marked as resolved.

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Similar suggestion for vertical white space, though, of course, there is only one EOL missing at the end of the file.

concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
@kotp
Copy link
Member

kotp commented Jan 6, 2022

Instead of the merge of main into this branch, it would be better to rebase this branch onto main. Feel free to do a force push to this branch as well, it is yours to work with.

@mathstrains21
Copy link
Contributor Author

mathstrains21 commented Jan 6, 2022

Instead of the merge of main into this branch, it would be better to rebase this branch onto main. Feel free to do a force push to this branch as well, it is yours to work with.

@kotp
For rebasing can I confirm that this is what I should do after pulling the changes into main?:

git checkout decorators
git rebase main

@mathstrains21 mathstrains21 force-pushed the decorators branch 5 times, most recently from e1f6969 to 80ca0df Compare January 6, 2022 10:31
@mathstrains21
Copy link
Contributor Author

mathstrains21 commented Jan 6, 2022

Hi! I have got to the point of returning a different function in decorators, and am struggling to decide how to introduce it.
Most decorators doing this would use *args and **kwargs, but I feel I don't want to use this syntax for the first example with the inner function. Could someone suggest a good introduction please?

Edit: Having once again read the RealPython article linked in #2356 I have an idea as to what to do. I will attempt to write it and will commit for feedback once done!
Edit2: (Now ready for feedback on this part)

@kotp
Copy link
Member

kotp commented Jan 6, 2022 via email

@mathstrains21 mathstrains21 force-pushed the decorators branch 7 times, most recently from ec61cf4 to ca00367 Compare January 7, 2022 18:47
@BethanyG BethanyG self-requested a review January 7, 2022 18:54
@mathstrains21
Copy link
Contributor Author

Is there any tool we are using that will validate the python code in code blocks?

Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

Overall, I think this needs some re-arranging for clarity. I'd also suggest dividing the decorator examples into categories -- so that you have an explicitly titled section for (these are just quick examples - not requests) decorators that take arguments and decorators that extend behavior vs decorators that change behavior or decorators that register, etc.

I also think this topic is complex enough that having real-world examples to anchor need/understanding is helpful. So while a few simple examples are good in the ramp-up, something like building up to the logging decorator Aaron Maxwell describes here I think would really help students understand the power of what they can do with them.

concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
@BethanyG
Copy link
Member

BethanyG commented Jan 7, 2022

Apologies, @mathstrains21 - I didn't see your comment from yesterday until just now, so my comments are for the whole thing, and not that particular section. 😱 I agree that the first example should be without *args and **kwargs -- but it is perfectly fine to have follow-on examples that do use them. I think our intent is to introduce *args and **kwargs in an exercise ahead of this one. But it is also fine to simply say here that they are a way of having arbitrary arguments, and to link to the docs as a way of explaining.

I did link to additional articles and authors that I think have some good decorator examples, but rather than have you plow through all the review comments, I will also just link them here:

David Beazley, in his Practical Python Programming course: function decorators
Bob Belderbos from PyBit.es: decorators by example & decorator optional arguments
Bob Belderbos on codementor.io: advanced python decorators
Aaron Maxwell on O'Riellys blog: 5 reasons you need to learn decorators

@BethanyG
Copy link
Member

BethanyG commented Jan 7, 2022

An additional article from Dan Bader, that has a good structure, I think: python-decorators

@BethanyG
Copy link
Member

BethanyG commented Jan 7, 2022 via email

@kotp
Copy link
Member

kotp commented Jan 8, 2022 via email

@mathstrains21
Copy link
Contributor Author

Apologies, @mathstrains21 - I didn't see your comment from yesterday until just now, so my comments are for the whole thing, and not that particular section. 😱 I agree that the first example should be without *args and **kwargs -- but it is perfectly fine to have follow-on examples that do use them. I think our intent is to introduce *args and **kwargs in an exercise ahead of this one. But it is also fine to simply say here that they are a way of having arbitrary arguments, and to link to the docs as a way of explaining.

Ok! That is roughly what I am doing anyway!

@mathstrains21
Copy link
Contributor Author

Overall, I think this needs some re-arranging for clarity. I'd also suggest dividing the decorator examples into categories -- so that you have an explicitly titled section for (these are just quick examples - not requests) decorators that take arguments and decorators that extend behavior vs decorators that change behavior or decorators that register, etc.

I also think this topic is complex enough that having real-world examples to anchor need/understanding is helpful. So while a few simple examples are good in the ramp-up, something like building up to the logging decorator Aaron Maxwell describes here I think would really help students understand the power of what they can do with them.

Have looked through your review, will review it in more detail this afternoon and start doing the rearranging!

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Requested change is the "decorater" typo. Others are suggestions, are, well, suggestive.

concepts/decorators/about.md Outdated Show resolved Hide resolved
concepts/decorators/about.md Outdated Show resolved Hide resolved
pass
```

If a decorator has defined default arguments, you must use parenthesis in the `@decorator()` call for the decorator to work:
Copy link
Member

Choose a reason for hiding this comment

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

Why? We might want to explain what the problem is being presented to the parser when there is no parenthesis. Or pose it as a "Getting to Deep Dive Python" point…

Copy link
Member

Choose a reason for hiding this comment

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

Next commit will include , as you would in calling any function:

concepts/decorators/about.md Outdated Show resolved Hide resolved
It could be coded to raise a `ValueError` instead.
So, the inner function wraps `func`, and returns either `func` or does something that substitutes what `func` would do.
The decorator returns its _inner function_.
It does not _directly_ return the original, passed-in function.
Copy link
Member

Choose a reason for hiding this comment

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

This suggests that it either must or may return the original passed in function indirectly. This could be clarified.

Copy link
Member

Choose a reason for hiding this comment

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

The last line will be changed in the next commit to

The inner_function may or may not return the original, passed-in function.

So, the inner function wraps `func`, and returns either `func` or does something that substitutes what `func` would do.
The decorator returns its _inner function_.
It does not _directly_ return the original, passed-in function.
Depending on what happens in the wrapper function, `func` may or may not be returned.
Copy link
Member

Choose a reason for hiding this comment

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

This does not, for me, clarify the prior sentence.

Copy link
Member

@bobahop bobahop Jun 16, 2022

Choose a reason for hiding this comment

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

The whole paragraph will be rephrased in the next commit to

The inner function returns either func or, if planet equals Pluto, it will print that Pluto is not a planet.
It could be coded to raise a ValueError instead.
So, the inner function wraps func, and returns either func or does something that substitutes what func would do.
The decorator returns its inner function.
The inner_function may or may not return the original, passed-in function.
Depending on what code conditionally executes in the wrapper function or inner_function, func may be returned, an error could be raised, or a value of func's return type could be returned.

It states and then repeats itself to hopefully be more reinforcing than obfuscatory.

concepts/decorators/about.md Outdated Show resolved Hide resolved
@bobahop
Copy link
Member

bobahop commented Jun 16, 2022

@kotp Thank you for your substantive observations! Please let me know if I have not addressed them successfully.

...
>>> my_func("Pluto")
Entering my_func with Pluto argument
Pluto is not a planet!
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants