-
-
Notifications
You must be signed in to change notification settings - Fork 874
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
Added 'notify' argument to the comment update() method #1008
Conversation
looks like the maintainers want to have the ci system back up and running before agreeing to any new merges. if you want to contribute to get yr pr through have a look here => #896 |
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.
could you please pull in the changes from master.
this should clean out the need for you change in tox.ini
@@ -608,14 +608,14 @@ def __init__(self, options, session, raw=None): | |||
if raw: | |||
self._parse_raw(raw) | |||
|
|||
def update(self, fields=None, async_=None, jira=None, body="", visibility=None): | |||
def update(self, fields=None, async_=None, jira=None, body="", visibility=None, notify=True): | |||
"""Update a comment""" |
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.
@ronniel1 , could you please also extend the documentation of this method to describe the parameters
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.
update from master
drop the changes to tox.ini
please extend the doc string for the update method to include parameters
@@ -99,7 +99,6 @@ commands = | |||
[testenv:lint] | |||
deps = pre-commit>=1.17.0 | |||
commands= | |||
bash -c "npm install && npm run spell" |
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 pull all changes from the master and drop this change
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.
Pleaser resolve current comments.
I'm taking this one up myself |
taking this up in #1387 |
The underlying update() method used, already supports the 'notify' argument.
Added that argument to Comment's update() method and propagated it on to Resource's update() method