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

Added 'notify' argument to the comment update() method #1008

Closed
wants to merge 1 commit into from

Conversation

ronniel1
Copy link

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

@studioj
Copy link
Collaborator

studioj commented Apr 4, 2021

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

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

@ssbarnea ssbarnea closed this May 3, 2021
@ssbarnea ssbarnea reopened this May 3, 2021
Copy link
Collaborator

@studioj studioj left a 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"""
Copy link
Collaborator

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

@studioj studioj closed this May 8, 2021
@studioj studioj reopened this May 8, 2021
Copy link
Collaborator

@studioj studioj left a 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"
Copy link
Collaborator

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

Copy link
Member

@ssbarnea ssbarnea left a 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.

@studioj studioj added the Status: Needs Rework PR has been submitted, but changes required. label Jul 21, 2021
@studioj
Copy link
Collaborator

studioj commented Jun 1, 2022

I'm taking this one up myself

@studioj studioj mentioned this pull request Jun 1, 2022
@studioj
Copy link
Collaborator

studioj commented Jun 1, 2022

taking this up in #1387

@studioj studioj closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Status: Needs Rework PR has been submitted, but changes required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants