Skip to content

Conversation

@penguinolog
Copy link
Contributor

  • Update classifiers
  • Update python_requires
  • Fix deprecated code (some from python 2.x):
    • io.open -> open
    • IOError -> OSError (in python 3 IOError is alias)
    • socket.error -> OSError
    • u'' -> ''
    • str().format -> f-strings
      (where it was quick automatically validated change)
    • object is not needed as base class
    • use set comprehensions
    • sum over generators (do not produce temporary lists)
    • use plain super()
  • remove unused imports

* Update classifiers
* Update python_requires
* Fix deprecated code (some from python 2.x):
  - `io.open` -> `open`
  - `IOError` -> `OSError`  (in python 3 `IOError` is alias)
  - `socket.error` -> `OSError`
  - `u''` -> `''`
  - `str().format` -> f-strings
    (where it was quick automatically validated change)
  - `object` is not needed as base class
  - use `set` comprehensions
  - `sum` over generators (do not produce temporary lists)
  - use plain `super()`
* remove unused imports
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

@penguinolog - thank you for taking the time to make these changes. There were still a few f-string conversions that we might as well get in if you don't mind.

@blink1073 - are these changes (drop of 3.6, add of 3.10) be something we could deliver in a 6.5 release?

def _upload(self, model, path):
"""Handle upload of a new file to path"""
self.log.info(u"Uploading file to %s", path)
self.log.info("Uploading file to %s", path)
Copy link
Member

Choose a reason for hiding this comment

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

f-string? (others below as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logging call without f-string is a bit more native (lazy logging formatting) and pylint will complain (BTW personally I prefer f-strings)

@blink1073
Copy link
Contributor

@blink1073 - are these changes (drop of 3.6, add of 3.10) be something we could deliver in a 6.5 release?

Makes sense to me

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

LGTM - thank you @penguinolog.

@penguinolog
Copy link
Contributor Author

@kevin-bates @blink1073 can this PR go forward?

@kevin-bates
Copy link
Member

Hi @penguinolog - yes, I'd like to move this forward but was hoping to get #6213 merged prior to avoid merge conflicts (although there's only a single file overlap in that PR). I've pinged the other PR so I'm hoping we get some traction there. Thank you for your patience.

@penguinolog
Copy link
Contributor Author

@kevin-bates looks like #6213 stuck with lack of frontenders :(

@Zsailer Zsailer changed the base branch from main to 6.4.x March 7, 2022 18:19
@echarles echarles added this to the 6.4 milestone Mar 20, 2022
@penguinolog
Copy link
Contributor Author

@kevin-bates can this PR go before frontend part? Project is moving forward and migration is stuck

@blink1073
Copy link
Contributor

@echarles since you're actively working on the 6.4.x, do you have an order preference?

@echarles
Copy link
Member

As the frontend releated changes will occur in the nbclassic repo jupyter/notebook-team-compass#5 (comment), this server change does not have to wait.

@kevin-bates @blink1073 Have you already looked at the changes, in which case I let you go forward. If not, I am happy to review.

@blink1073
Copy link
Contributor

Kevin already reviewed, so proceeding. Thanks @penguinolog!

@blink1073 blink1073 merged commit 4ba07e1 into jupyter:6.4.x Apr 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants