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

Different bash function syntaxes are tolerated #32

Closed
f18m opened this issue Jan 8, 2019 · 7 comments · Fixed by #39
Closed

Different bash function syntaxes are tolerated #32

f18m opened this issue Jan 8, 2019 · 7 comments · Fixed by #39

Comments

@f18m
Copy link
Contributor

f18m commented Jan 8, 2019

Hi,
I noticed that beautysh is not enforcing any specific style for the function declarations:

A)

function myfunc()
{
    ...
}

is accepted as well as
B)

function myfunc
{
    ...
}

and as well as
C)

myfunc()
{
    ...
}

I think beautysh should stick with one and enforce it.
What do you think?

@lovesegfault
Copy link
Owner

Hmm, I think this is a little tricky because these are all distinct syntaxes, but with the same style. I'm not sure whether it is beautysh's place to be picky about syntax (which would make it a very opinionated formatter), rather than just style/form.

Thoghts? :)

@f18m
Copy link
Contributor Author

f18m commented Jan 9, 2019

Well actually tools like
https://clang.llvm.org/docs/ClangFormat.html
is quite picky and strict and e.g. does not allow you to mix different function declaration styles in C/C++... I'm using it a lot on C/C++ projects exactly because it enforces all developers working on the same sources to stay coherent to some style guidelines (whatever they are).

In case quoted above I think that honestly it is confusing to have inside the same Bash file option A, B or C mixed together... so I would love to have a tool that simply enforces one and only one of them (I would vote A)...

Of course the best would be to have a config option to choose your preferred style...

@lovesegfault
Copy link
Owner

lovesegfault commented Jan 10, 2019

@f18m So wait, are you suggesting:

  1. That we enforce consistency (you can use either style A, B, or C but not mix them) [or]
  2. That we enforce style (you should use K=[A|B|C] as opposed to [A,B,C]\K

I'm all for 1, but I'm still not sold on 2. Which style do you think we should enforce of the ones you listed?

@f18m
Copy link
Contributor Author

f18m commented Jan 10, 2019

I actually would be fine with both 1) and 2).
In solution 1) I think that beautysh should however have some logic for autodetecting the currently-in-use style. In solution 2) it's the user that explicitly tells beautysh which style to enforce (so perhaps a bit easier to implement).

Btw IMHO the best style for Bash function is A). It's also the longest to type but once you configure your editor to run beautysh and it's beautysh that does the reformatting work for you, then who cares what's longer to type :)

PS: I'm working with the author of https://github.com/de-jcup/eclipse-bash-editor to integrate external reformatters like beautysh in that Eclipse plugin :)
See feature #117 in that project.

@lovesegfault
Copy link
Owner

@f18m Writing a script today I actually encountered this issue! (see if you can spot the inconsistency :P )

I am all for 2) with a defaulting behavior to A. 1) can be implemented later based on 2) I believe.

If this is implemented I will happily merge it :)

@lovesegfault
Copy link
Owner

@f18m I just cut a new release (3.12) with all of the changes you've been making. If this eventually gets built I will add it to a 4.0 release :)

@f18m
Copy link
Contributor Author

f18m commented Jan 16, 2019

@bemeurer great, I'm looking forward to test version 3.12 in my "production" :)

Btw I just cleaned up a PR for this issue... maybe it can still be improved a little bit, but should be a good start...

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 a pull request may close this issue.

2 participants