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

Add themes/base.theme.bash to clean files #1785

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

marcospereira
Copy link
Contributor

@marcospereira marcospereira commented Jan 11, 2021

Description

Review with ?w=1.

  • Add themes/base.theme.bash to clean files
  • Run code fomatter for themes/base.theme.bash
  • Fix shellcheck warnings for themes/base.theme.bash
  • Fix shellcheck header script to consider multiple lines

Motivation and Context

I've picked this file because it is the file with most changes (commits, at least) based on:

git log --name-only --pretty="format:" | grep -v -e "^[[:space:]]*$" | sort | uniq -c | sort

I thought about adding other files, but that would probably make it harder to review.

How Has This Been Tested?

Besides running the bats tests locally, I've manually tested that the most significant in isolation (checking they are resulting in the same effect as before).

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • File linting

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

hooks/dot-bash.sh Outdated Show resolved Hide resolved
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Wow @marcospereira, amazing job 😄
I will take a look in the weekend, but I want to say thank you for taking your time to put an effort to improve bash it ❤️

@marcospereira
Copy link
Contributor Author

Thanks for reviewing, @NoahGorny and @davidpfarrell. I've removed the changes in dots-bash.sh.

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Amazing work @marcospereira!
Thank you for your effort!

@NoahGorny NoahGorny merged commit a9b0ec7 into Bash-it:master Jan 13, 2021
@marcospereira marcospereira deleted the lint/themes/base.theme.bash branch January 13, 2021 19:32
@marcospereira
Copy link
Contributor Author

Thanks, @NoahGorny. So, I can invest some more time in this cleanup task, but perhaps just prioritizing files that have many changes isn't the better way to do it, right?

Are there any specific areas where you think it would be more relevant now?

@NoahGorny
Copy link
Member

Thanks, @NoahGorny. So, I can invest some more time in this cleanup task, but perhaps just prioritizing files that have many changes isn't the better way to do it, right?

Are there any specific areas where you think it would be more relevant now?

I think that going after the core files, like the file you fixed here, is a great idea. Just make sure there is no pending PRs for the file, and lint away 😄
For hints, I think bash_it.sh is a challenging file worth cleaning up, also the lib directory contains important files.
Thank you for doing this @marcospereira !

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.

3 participants