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

[WIP]Update close method to cpdef #122

Draft
wants to merge 5 commits into
base: devel
Choose a base branch
from

Conversation

ganeshrn
Copy link
Member

@ganeshrn ganeshrn commented Aug 4, 2020

SUMMARY

Define close() as cpdef

ISSUE TYPE
  • Bugfix Pull Request
ADDITIONAL INFORMATION

@ganeshrn ganeshrn changed the title Update close method to cpdef [WIP]Update close method to cpdef Aug 4, 2020
@webknjaz webknjaz marked this pull request as draft August 4, 2020 13:45
Comment on lines +111 to +119
def __del__(self):
self.close()
Copy link
Member

Choose a reason for hiding this comment

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

Why do you think it's better to use __del__ in Python-land?

@@ -46,7 +46,7 @@ cdef int _process_outputs(libssh.ssh_session session,
return len

cdef class Channel:
def __cinit__(self, session):
Copy link
Member

Choose a reason for hiding this comment

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

why?

@webknjaz
Copy link
Member

webknjaz commented Aug 4, 2020

@ganeshrn what is it that you're trying to fix here?

@webknjaz
Copy link
Member

@ganeshrn is this still relevant?

@webknjaz webknjaz added the help wanted Extra attention is needed label Jun 29, 2021
@webknjaz webknjaz force-pushed the close_fix branch 2 times, most recently from 169ed04 to aa1ad0a Compare September 18, 2022 17:07
@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/ansible-pylibssh-122
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@webknjaz
Copy link
Member

@Jakuje would you mind looking into this patch? I'm not able to figure out the motivation behind it, but perhaps you could?

@Jakuje
Copy link
Contributor

Jakuje commented Nov 20, 2024

@Jakuje would you mind looking into this patch? I'm not able to figure out the motivation behind it, but perhaps you could?

Sorry, I have no idea. It looks like just moving things around without good explanation. Given that we did not hear from the reporter for 4 years, I would not bother.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants