-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Issue 2034 #2088
Issue 2034 #2088
Conversation
@@ -190,7 +190,9 @@ def __getattr__(self, name): | |||
|
|||
def compatproperty(name): | |||
def fget(self): | |||
# deprecated - use pytest.name | |||
import warnings | |||
warnings.warn("compatproperty is deprecated. Use pytest.name", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be: "compatproperty is deprecated. Use pytest.{0}".format(name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm actually this probably should be:
"this usage is deprecated, please use pytest.{0} instead".format(name)"
because I'm not sure of all cases this is deprecating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the name of the property to the deprecation message.
Thanks! |
Also please rebase on |
Argh, sorry for the typo in the commit message! |
Feel free to amend it. I just noticed that the CHANGELOG message could also
mention that what's being deprecated is accessing item.Function,
item.Module, etc. If you amend the commit could you update the CHANGELOG as
well? If not, don't worry about it, I can do it later
…On Sat, Nov 26, 2016, 18:40 Neven Munđar ***@***.***> wrote:
Argh, sorry for the typo in the commit message!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2088 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABCO_NTP4ILE8PRt3Rc2yTdwjvAH-Oevks5rCJlQgaJpZM4K837e>
.
|
Please, if it's not a bother, update the changelog as you see fit, you have a better idea about wording. Don't want to mess up, the same with force pushing amended commits. |
Sure, I will do it before merging, thanks!
…On Sat, Nov 26, 2016, 19:06 Neven Munđar ***@***.***> wrote:
Please, if it's not a bother, update the changelog as you see fit, you
have a better idea about wording. Don't want to mess up, the same with
force pushing amended commits.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2088 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABCO_PK_KJkdGdcGnIdIylRXBYhJJkvEks5rCJ9FgaJpZM4K837e>
.
|
1b31521
to
90fa48c
Compare
90fa48c
to
2d71979
Compare
Updated CHANGELOG and rebased on |
Thanks for submitting a PR, your contribution is really appreciated!
Here's a quick checklist that should be present in PRs:
master
; for new features, targetfeatures
;Unless your change is trivial documentation fix (e.g., a typo or reword of a small section) please:
AUTHORS
;CHANGELOG.rst
CHANGELOG
, so please add a thank note to yourself ("Thanks @user for the PR") and a link to your GitHub profile. It may sound weird thanking yourself, but otherwise a maintainer would have to do it manually before or after merging instead of just using GitHub's merge button. This makes it easier on the maintainers to merge PRs.