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

Backport to Python 2.7 #165

Merged
merged 6 commits into from
Aug 9, 2022
Merged

Backport to Python 2.7 #165

merged 6 commits into from
Aug 9, 2022

Conversation

herronelou
Copy link

Reading #3, it looks like at least at one point there was a desire to support python 2.7.
While it's meant to be dead and buried right now, I've had to add support for it today for usage along with some other libraries and compiled programs that haven't been updated yet.

Seeing as the changes required to make it work in python 2.7 were very minimal, maybe you would like to merge it.
All that was required was replacing the non-ascii apostrophe used in some docstrings with an ascii apostrophe, and replacing a few f-strings with str.format().

Copy link
Owner

@jadolg jadolg left a comment

Choose a reason for hiding this comment

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

Please add python 2.7 back to the tests over here

python-version: [ '3.6', '3.7', '3.8', '3.9' ]

and correct the problems found by the CI

@jadolg
Copy link
Owner

jadolg commented Jul 6, 2022

I decided to stop supporting 2.7 back when it EoLed but if there's someone using it makes sense.

@herronelou
Copy link
Author

Hello, thanks for the feedback.
Concerning the issues raised by the CI, one of the issue it complains about is that I'm not using f-strings, which is the point of the change, I'll look into how to disable this warning,
The others were about lines too long, one of which I've introduced, the others were already here in the docstring, do you want me to reformat the ones for which I only changed the apostrophe?

@jadolg
Copy link
Owner

jadolg commented Jul 7, 2022

Hello, thanks for the feedback. Concerning the issues raised by the CI, one of the issue it complains about is that I'm not using f-strings, which is the point of the change, I'll look into how to disable this warning, The others were about lines too long, one of which I've introduced, the others were already here in the docstring, do you want me to reformat the ones for which I only changed the apostrophe?

Looks like deepsource only gets triggered for changes and it was introduced after we wrote those docstrings. I'll fix that at some point. Don't pay too much attention to deepsource but to the Test and Publish workflow (https://github.com/jadolg/rocketchat_API/runs/7219372176?check_suite_focus=true). Please start by formatting the files you changed with black

@herronelou
Copy link
Author

Hi @jadolg.
Sorry for the delay, I've had other things to deal with at work that took priority over attempting to fix this.

I've enabled the actions on my fork so that I could try to get it passing by adding 2.7 to the test matrix, I've had to do a small tweak to the actions as black doesn't run on 2.7.

However, the tests fail but they also fail using your master at the same stage:
https://github.com/jadolg/rocketchat_API/actions/runs/2598852580

You're likely a lot more familiar with the RocketChat API than I am, maybe you know how to fix this one.

@jadolg
Copy link
Owner

jadolg commented Jul 12, 2022

Hi @herronelou
fantastic! I'm aware of the failure of the tests. We always test against the latest rocket.chat version and looks like they changed something upstream that made this method fail. I already posted about it over here https://open.rocket.chat/channel/python_rocketchat_api but I have not had answers yet.

@jadolg
Copy link
Owner

jadolg commented Jul 16, 2022

If you are interested in following up, here's the ticket on rocket.chat. RocketChat/Rocket.Chat#26262

@jadolg
Copy link
Owner

jadolg commented Aug 9, 2022

hey @herronelou 👋
Would you mind rebasing against master?
The issue should be gone already.

EDIT: nevermind, GitHub allowed me to do it

@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #165 (2f33caf) into master (65ce2b0) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #165   +/-   ##
=======================================
  Coverage   97.66%   97.66%           
=======================================
  Files          37       37           
  Lines        1542     1542           
  Branches      127      127           
=======================================
  Hits         1506     1506           
  Misses         31       31           
  Partials        5        5           
Impacted Files Coverage Δ
rocketchat_API/APISections/channels.py 100.00% <ø> (ø)
rocketchat_API/APISections/groups.py 100.00% <ø> (ø)
rocketchat_API/APISections/im.py 100.00% <ø> (ø)
rocketchat_API/APISections/users.py 96.92% <ø> (ø)
tests/test_user.py 96.58% <ø> (ø)
rocketchat_API/APISections/livechat.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jadolg jadolg merged commit 0cd5dab into jadolg:master Aug 9, 2022
@jadolg
Copy link
Owner

jadolg commented Aug 9, 2022

I'll be releasing this today

@herronelou
Copy link
Author

Thanks! Sorry for the delay, I was away at a conference all week.

@herronelou
Copy link
Author

herronelou commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants