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

Using TAB escape sequence instead of pure spaces #1353

Merged
merged 3 commits into from
Apr 12, 2022
Merged

Conversation

lnxpy
Copy link
Contributor

@lnxpy lnxpy commented Apr 2, 2022

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

  • Using the escape sequences instead of spaces.
  • Using isort in order to sort imports in the entire project.

Rationale

In some cases, like when you want to stream the STDOUT and you are about the stream a huge amount of data like log records, using the pure spaces might break the responsiveness in the CLI. Better approach is to use sequences. In spite, I found some escape sequences like \n in the code, but there were pure spaces (4 spaces) instead of \t. It also helps for better readability.

Testing/Review Recommendations

Please make sure you run a full test. Despite of isort's power, there still might be some special cases that isort may cause of import errors and stuff. However, I've never faced any of them.

Future Work

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Looks good to me. My guess is the python formatter is going to break on the changes where lines have collapsed.... but this is for CI to catch.

In the future, I'd suggest splitting such PRs into two, as the original (tabs) was easy to review, but the larger one at 62 files took much longer to review.

Regardless, thanks for the PR. We'll see how CI does.

@lnxpy
Copy link
Contributor Author

lnxpy commented Apr 11, 2022

Thank you. I actually started using escape sequences in only one file from the project to begin this methodology of logging or even printing stuff to the CLI in a better scheme and shape. Almost all those 61 files are being modified because I tried to sort the import lists of the entire project and I highly believe that CI failure must be due to second commit.

@LeStarch
Copy link
Collaborator

Closing and reopening as that seems to force a full rerun of the failing CI.

@LeStarch LeStarch closed this Apr 11, 2022
@LeStarch LeStarch reopened this Apr 11, 2022
@LeStarch
Copy link
Collaborator

@lnxpy it might be, but we've had a failing CI for a week that was resolved only today. Thus I am rerunning all things before I take the failures seriously.

@LeStarch
Copy link
Collaborator

@Inxpy yes, now the formatter is balking because the tool you ran changed the format. To fix, I recommend you run:

pip install click==8.0.4 black==21.6b0
cd fprime
black .

That should clean up the files and fix the issue! It will still keep the sorted imports, but will correct the collapsed lines. Let me know if you need help running this!

@lnxpy
Copy link
Contributor Author

lnxpy commented Apr 12, 2022

@LeStarch Would you please close and reopen for another CI rerun?

@LeStarch
Copy link
Collaborator

This is great work. Thanks for getting this prepared!

@LeStarch LeStarch merged commit edc2f1c into nasa:devel Apr 12, 2022
LeStarch pushed a commit that referenced this pull request Jun 29, 2022
* using \t instead if raw spaces

* isort observed

* black observed
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