The Wayback Machine - https://web.archive.org/web/20200921061706/https://github.com/repobee/repobee/issues/460
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

Replace str.format with f-strings #460

Open
slarse opened this issue Jul 9, 2020 · 6 comments
Open

Replace str.format with f-strings #460

slarse opened this issue Jul 9, 2020 · 6 comments

Comments

@slarse
Copy link
Collaborator

@slarse slarse commented Jul 9, 2020

IMPORTANT: Doing this for the entire codebase in a single PR would be infeasible to review, and would most likely lead to a large amount of conflicts. Do one module per PR, and reference this issue in the PR (but don't say "fix", just reference the issue).

As we have finally dropped support for Python 3.5, we can now utilize f-strings. It would be a good idea to replace format strings with f-strings, except in the places where we use format strings as templates that are stored in variables. There is a TON of use of str.format in the test suite in particular.

So, all instances on the form "some literal {} string".format(var) should be replaced with f"some literal {var} string". Here's an example of where we should use an f-string instead.

description="{} created for {}".format(
repo_base_name, student
),

The above should be rewritten as:

description=f"{repo_base_name} created for {student}"

Finding modules with str.format calls: Simply executing grep -r \.format in a shell should net you most occurrences.

Progress (i.e. don't start working on these modules because someone else is):

  • _repobee.ext.gitlab: #470 (DONE)
@slarse
Copy link
Collaborator Author

@slarse slarse commented Jul 9, 2020

@tohanss This would be a good issue for you to start poking around the codebase!

@ShreyaDhananjay
Copy link
Contributor

@ShreyaDhananjay ShreyaDhananjay commented Jul 9, 2020

Hi can I start working on this?

@slarse
Copy link
Collaborator Author

@slarse slarse commented Jul 9, 2020

@ShreyaDhananjay Hi and welcome!

You're absolutely welcome to start at this, it would be much appreciated. The worst offenders in using str.format in the production code are the github module and the gitlab module. The test suite also makes extensive use of it.

Since this is such a broad issue, it would be good if you indicate either by a comment in this issue or by PRs which module(s) you're working on.

Ping me if you need any help :)

@ShreyaDhananjay
Copy link
Contributor

@ShreyaDhananjay ShreyaDhananjay commented Jul 9, 2020

I'll start working on the github and gitlab modules then. Thanks!

@slarse
Copy link
Collaborator Author

@slarse slarse commented Jul 9, 2020

@ShreyaDhananjay Excellent!

Oh, and fair warning, black is pretty unhelpful when it comes to formatting strings (read: barely helps at all), so you will likely have to do a decent amount of manual formatting in order to satisfy the 79 character line length requirement. Don't hesitate to submit a partially complete PR if you can't quite make that work, and we'll just work it out.

@ShreyaDhananjay
Copy link
Contributor

@ShreyaDhananjay ShreyaDhananjay commented Jul 9, 2020

Okay thanks!

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.