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

Move up to fstrings #1886

Merged
merged 21 commits into from Nov 29, 2018
Merged

Move up to fstrings #1886

merged 21 commits into from Nov 29, 2018

Conversation

DrFaustie
Copy link
Member

@DrFaustie DrFaustie commented Nov 17, 2018

Fixes #1659

For when it's necessary to drop support for older Python Versions.

  • kinto/config/__init__.py
  • kinto/schema_validation.py
  • kinto/core/scripts.py
  • kinto/core/openapi.py
  • kinto/core/__init__.py
  • kinto/core/utils.py
  • kinto/core/cache/postgresql/__init__.py
  • kinto/core/storage/memory.py
  • kinto/core/storage/exceptions.py
  • kinto/core/storage/postgresql/client.py
  • kinto/core/storage/testing.py
  • kinto/core/statsd.py
  • kinto/core/initialization.py
  • kinto/core/resource/model.py
  • kinto/core/resource/schema.py
  • kinto/core/permission/memory.py
  • kinto/core/views/heartbeat.py
  • kinto/core/views/errors.py
  • kinto/core/views/batch.py
  • kinto/core/testing.py
  • kinto/plugins/quotas/scripts.py
  • kinto/plugins/quotas/listener.py
  • kinto/plugins/openid/__init__.py
  • kinto/plugins/openid/views.py
  • kinto/plugins/default_bucket/__init__.py
  • kinto/plugins/history/listener.py
  • kinto/authorization.py
  • kinto/views/records.py
  • kinto/core/authentication.py
  • kinto/core/events.py
  • kinto/core/storage/postgresql/__init__.py
  • kinto/core/storage/postgresql/migrator.py
  • kinto/core/errors.py
  • kinto/core/resource/__init__.py
  • kinto/core/resource/viewset.py
  • kinto/core/permission/postgresql/__init__.py
  • kinto/core/authorization.py
  • kinto/plugins/accounts/scripts.py
  • kinto/plugins/accounts/__init__.py
  • kinto/plugins/accounts/views.py
  • kinto/plugins/accounts/authentication.py
  • kinto/views/permissions.py
  • kinto/views/groups.py
  • kinto/__main__.py

Cleanup

  • Add documentation.
  • Add tests.
  • Add a changelog entry.

@DrFaustie
Copy link
Member Author

DrFaustie commented Nov 17, 2018

There may be some issues still. My local builds still fail on lint and functional ENV's.
https://i.imgur.com/6UrNgXI.png

Perhaps it is correct for the functional build to fail as it runs on 3.5?

There are also some lines of code that I'm unsure of on how to convert to fstrings at the moment. I'll look into those over the next few days. Code review/advice is very much appreciated

@@ -34,6 +34,7 @@ def unauthenticated_userid(self, request):
return

hmac_secret = settings["userid_hmac_secret"]
# Unsure how to use fstring with *args
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that f"{credentials[0]}:{credentials[1]" is fine. Or leave the format() call

@@ -29,6 +29,7 @@ def __init__(self, payload, request):
self.request = request

def __repr__(self):
# Not sure what proper conversion to fstring is
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably better to leave it with format()

Copy link
Member Author

@DrFaustie DrFaustie Nov 20, 2018

Choose a reason for hiding this comment

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

We could do this, I think it should work

Suggested change
# Not sure what proper conversion to fstring is
# def __repr__(self):
return f"<{self.__class__.__name__} action={self.payload['action']} uri={self.payload['uri']}>"

@leplatrem
Copy link
Contributor

It looks to me! Thanks for taking the time to do this!

Perhaps it is correct for the functional build to fail as it runs on 3.5?

This PR will drop support of Python 3.5.
The TravisCI configuration and CHANGELOG should also be updated to reflect that ;)

It looks like you should run black to format your changes (see TravisCI job py36)

@DrFaustie
Copy link
Member Author

It's my pleasure!

About the TravisCI config, we should update the python version but also tell Travis that we expect 3.5 to fail, correct?

@Natim
Copy link
Member

Natim commented Nov 20, 2018

tell Travis that we expect 3.5 to fail

No we should remove the travis config for py35 altogether.

@Natim Natim requested a review from leplatrem November 20, 2018 15:26
@Natim
Copy link
Member

Natim commented Nov 20, 2018

Thank you for this work. To be honest I didn't expect it to be so much of an improvement.
👍 🎱

@DrFaustie
Copy link
Member Author

Thank you for this work. To be honest I didn't expect it to be so much of an improvement. 👍 🎱

You and @leplatrem have been very kind to me and made my first experience with open source extremely welcoming. I'm very happy to have started here at Kinto 😀

@Natim
Copy link
Member

Natim commented Nov 20, 2018

Let's make a quick poll: https://twitter.com/Natim/status/1064904568272764931

@DrFaustie
Copy link
Member Author

DrFaustie commented Nov 20, 2018

Let's make a quick poll: https://twitter.com/Natim/status/1064904568272764931

I think you may need to edit your link

@Natim
Copy link
Member

Natim commented Nov 20, 2018

I did indeed 💃

@Natim
Copy link
Member

Natim commented Nov 27, 2018

10 days later:
image

@DrFaustie
Copy link
Member Author

Looks good!

@leplatrem
Copy link
Contributor

Could you please update your branch, we'll merge ;)

@DrFaustie
Copy link
Member Author

Is there anything that still needs to be done from me? 👍

@Natim
Copy link
Member

Natim commented Nov 29, 2018

Nope it seems good to me 👍

Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

👍

@leplatrem leplatrem merged commit ea5f66d into Kinto:master Nov 29, 2018
@DrFaustie
Copy link
Member Author

Changelog is not properly dated on 12.0.0

@glasserc
Copy link
Contributor

It's still unreleased and it's marked as such, so I think it's OK, right?

@Natim
Copy link
Member

Natim commented Nov 30, 2018

Yes it is the normal process, the date is fixed only when we release it.

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.

None yet

4 participants