Skip to content

Conversation

@casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Jan 30, 2020

  • whitespace formatting (fixes add shfmt hooks to format shell scripts #2834 automate formatting)
  • consistent use of print_error()/echo/echo -e/echo >&2
  • possibly abstract common functionality into one place
  • add pre-commit hook
    • ensure CI tests work
  • discuss & document go>=1.13 requirement use python formatter!
  • fixes tidy scripts #2969

if reviewing online, make sure to ignore whitespace to see actual changes quicker

image

@casperdcl casperdcl added refactoring Factoring and re-factoring testing Related to the tests and the testing infrastructure labels Jan 30, 2020
@casperdcl casperdcl requested a review from efiop January 30, 2020 22:45
@casperdcl casperdcl self-assigned this Jan 30, 2020
@efiop
Copy link
Contributor

efiop commented Jan 31, 2020

consistent use of print_error()/echo/echo -e/echo >&2

possibly abstract common functionality into one place

Let's not bother with those for now, they have a very low priority.

@casperdcl casperdcl marked this pull request as ready for review January 31, 2020 18:47
@casperdcl
Copy link
Contributor Author

Oh also just found https://github.com/lovesegfault/beautysh which is python (yay) but does say

in tests with large Linux system Bash scripts, its error-free score was ~99%

which sounds scary. It's ambiguous whether it means it'll break scripts, or just break formatting.

@casperdcl casperdcl changed the title lint:shfmt pre-commit hook lint:beautysh pre-commit hook Jan 31, 2020
@casperdcl
Copy link
Contributor Author

casperdcl commented Jan 31, 2020

which sounds scary. It's ambiguous whether it means it'll break scripts, or just break formatting.

Got confused with one script:

[[ condition1 && \
   condition2 ]]

where it fixed indentation in such a way that it kept complaining about its own formatting.

I manually fixed this in a way that shouldn't be a problem.

@efiop
Copy link
Contributor

efiop commented Jan 31, 2020

@casperdcl That is pretty scary. Would rather not rely on something so unreliable, as it might cause big problems for an unsuspecting contributor.

@casperdcl
Copy link
Contributor Author

well idk... it didn't break anything and also is used in https://github.com/Glavin001/atom-beautify. Seems like it'll solve a lot more problems than it may introduce. I know it hasn't broken anything so far because the diff between shfmt and beautysh is 4e87bf9. I don't like shfmt's go dependency any more than @shcheklein :)

@efiop
Copy link
Contributor

efiop commented Jan 31, 2020

@casperdcl To be honest, still quite worried about it, but I am willing to give it a try for now. If anything goes wrong - we'll just get rid of it. Thanks!

@efiop efiop merged commit 2613e12 into iterative:master Jan 31, 2020
@casperdcl casperdcl deleted the shfmt branch January 31, 2020 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Factoring and re-factoring testing Related to the tests and the testing infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tidy scripts add shfmt hooks to format shell scripts

3 participants