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

Force finish (stopping) launch added #22

Merged
merged 3 commits into from
Mar 14, 2018

Conversation

tmarenko
Copy link
Contributor

No description provided.

@@ -139,6 +139,17 @@ def finish_launch(self, end_time, status=None):
logger.debug("finish_launch - Stack: %s", self.stack)
return _get_msg(r)

def stop_launch(self, end_time, status=None):
Copy link
Contributor

@krasoffski krasoffski Mar 12, 2018

Choose a reason for hiding this comment

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

stop_launch is almost the same as finish_launch except s/finish_/stop_/.
It would be great to create a common method like _finalize_launch or any other name you like with semantics _finalize_launch(self, end_time, status=None, method="finish"). But it is required to check clients which use this lib because of semantics changes. What do you think about that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@krasoffski I think that it is better to follow the original API while doing such packages to work with it. Mapping method to method looks much more clear than some mixed method doing multiple things. So the idea not in functionality of the package itself but in following functionality of the base product.

So here I would stay this code as it is now.

@ailjushkin
Copy link
Collaborator

@tmarenko also you need to update the package version to be able to upload it on pypi.

@ailjushkin
Copy link
Collaborator

ailjushkin commented Mar 13, 2018

@tmarenko allright, I have removed my comment about the stack pop. Let's stay it as it is. But you need to bump the version to be able to upload that package on pypi.

ailjushkin
ailjushkin previously approved these changes Mar 14, 2018
Copy link
Collaborator

@ailjushkin ailjushkin left a comment

Choose a reason for hiding this comment

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

Thanks.

Copy link
Contributor

@krasoffski krasoffski left a comment

Choose a reason for hiding this comment

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

@tmarenko, @ailjushkin Thank you for you effort. There is one more option to reduce copy&paste for now, just an example:

def _finalize_launch(self, end_time, action, status):
    data = {
        "end_time": end_time,
        "status": status
    }
    url = uri_join(self.base_url, "launch", self.launch_id, action)
    r = self.session.put(url=url, json=data)
    self.stack.pop()
    logger.debug("%s_launch - Stack: %s", action, self.stack)
    return _get_msg(r)

And then two non-private methods like:

 def finish_launch(self, end_time, status=None):
    return self._finalize_launch(end_time=end_time, action="finish", status=status)

and

 def stop_launch(self, end_time, status=None):
    return self._finalize_launch(end_time=end_time, action="stop", status=status)

Could you please try this solution?

@krasoffski
Copy link
Contributor

Sorry for my severity.

@ailjushkin
Copy link
Collaborator

@krasoffski sounds awesome, thanks

Copy link
Contributor

@krasoffski krasoffski left a comment

Choose a reason for hiding this comment

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

Despite the fact that it was possible to perform the similar changes for service_async.py file, it's not worth it to save two lines of code.

@krasoffski
Copy link
Contributor

@avarabyeu, @DzmitryHumianiuk
Could you please merge this request?

@DzmitryHumianiuk DzmitryHumianiuk merged commit cd0509b into reportportal:master Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants