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

Codebase formatting with f-string #50

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Codebase formatting with f-string #50

merged 1 commit into from
Apr 26, 2022

Conversation

ThibFrgsGmz
Copy link
Contributor

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

  • Replace calls to string.format() with f-strings ref
  • Replace the use of the % string interpolation operator with f-strings (only %s operator) ref

Rationale

According to the Python documentation, using the % operator to format strings can lead to "a variety of quirks that lead to a number of common errors."

PEP 498 added F-strings to Python in version 3.6. F-strings are a versatile and powerful method of string formatting. Because the code looks more like the output, they make it shorter and more readable.

Testing/Review Recommendations

Future Work

Deals with operators other than %s like %d.

@ThibFrgsGmz
Copy link
Contributor Author

@LeStarch Damn, I was testing my fork with linters. As I broke a lot of things on my fork,
I wanted to start from a healthy base and delete it and the fork...
I don't know if it's possible to recover the commit.

@LeStarch
Copy link
Collaborator

The commit is still out there. Recovering it is the issue.

@LeStarch LeStarch reopened this Apr 19, 2022
@LeStarch
Copy link
Collaborator

Look here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

If you checkout the merged pull-request branch, it should exist. Then you can make that a new branch.

Or if you prefer, follow those instructions to fetch the PR. Then from a clean codebase:

git checkout -b somebranch <upstream>/devel
git cherry-pick 9724f6e95cd32890e478b72283cf0e6850af4ccb

This will migrate over exactly the above commit into a new off-devel branch

@LeStarch
Copy link
Collaborator

I am not sure this will work, but I think it will. You should do these steps soon as eventually the repository will be garbage-collected and it will disappear.

@ThibFrgsGmz
Copy link
Contributor Author

Ok great thanks! I was able to retrieve the commits from my PR #51 #52 #53.
I will now see how to repatriate them in my fork and submit them via other PRs.

@ThibFrgsGmz
Copy link
Contributor Author

@LeStarch Since you were able to reopen it, do you have the option to merge it? In that case, I don't need to redo the PR and let you look at my different PRs, right ?

@LeStarch
Copy link
Collaborator

I can, but it looks like CI still needs some fixes.

@ThibFrgsGmz
Copy link
Contributor Author

@LeStarch Ok, there seems to be a problem on Black and then it seems to be related to the FPP installation.
Do you want me to look into fixing the CI or do you want to wait until you finish the new FPrime install procedure via pip to fix it?

@LeStarch
Copy link
Collaborator

I have a fix for the Black/CI install issue.

@LeStarch LeStarch closed this Apr 26, 2022
@LeStarch LeStarch reopened this Apr 26, 2022
@LeStarch LeStarch merged commit 0fd1d05 into nasa:devel Apr 26, 2022
@LeStarch
Copy link
Collaborator

Only format not working. I am going to merge and push a reformat as a separate PR. This is to not lose the changes.

@ThibFrgsGmz
Copy link
Contributor Author

@LeStarch Okay, nice what about my other PRs that suffered the same closing fate? Can you reopen them?

@LeStarch
Copy link
Collaborator

Sorry, I did not realize there were more.

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

2 participants