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

Should *_test.sh be a bats file? #405

Closed
tqa236 opened this issue Nov 17, 2019 · 11 comments · Fixed by #541
Closed

Should *_test.sh be a bats file? #405

tqa236 opened this issue Nov 17, 2019 · 11 comments · Fixed by #541

Comments

@tqa236
Copy link

tqa236 commented Nov 17, 2019

Because, as far as I understand, it's a bats file...

bats files doesn't work very well with the tools for sh file, like shellcheck or kcov, at least to me.

Should we change the extension to .bats to show that they are bats files?

@IsaacG
Copy link
Member

IsaacG commented Nov 28, 2019

+1

Personally, I'm in favor of getting rid of all .sh extensions ... unless we're doing POSIX sh. The executables in /bin don't have extensions so I don't see why executable shell scripts should have 'em. If we do need them, there's .bash and .bats

@glennj
Copy link
Contributor

glennj commented Nov 28, 2019

I don't have strong opinions about this. The Google shell style guide has this to say about it

Executables should have no extension (strongly preferred) or a .sh extension. Libraries must have a .sh extension and should not be executable.

It is not necessary to know what language a program is written in when executing it and shell doesn't require an extension so we prefer not to use one for executables.

However, for libraries it's important to know what language it is and sometimes there's a need to have similar libraries in different languages. This allows library files with identical purposes but different languages to be identically named except for the language-specific suffix.

We can consider the test scripts as libraries, so I can see them with a .bats extension if we either remove the shebang or change it to #!/usr/bin/env bats

The script files are not by default executable: the tests explicitly pass the filename to bash. I suppose I'd prefer dropping the extension rather than using .bash.

But I'm personally not motivated enough to do the work.

@tqa236
Copy link
Author

tqa236 commented Nov 29, 2019

I can make a PR if we can all agree on how to change them. I have one concern though. How will the CLI behave if we change the extensions of the files? I believe they will just download new files without deleting the old files. Is this correct?

@guygastineau
Copy link
Contributor

@tqa236 I think you are right about how the CLI will behave.

There is a PR up right now that has to do with file names, but that work was automated, and a fix could be automated for it that wouldn't make it horrible to fix. You are right to think that older solutions will leave a weird directory when they are updated.

If we agree to use .bats for the test files, and no extension for the solution files, then that works for me. Most people will be structuring the solutions as executable files. If we want the .bash suffix on the solution files that is also fine with me (even though that should be fore libraries). I have wanted the extensions to move away from sh for quite some time now anyway.

If you want to make a PR for this it would be appreciated. Navigating all of the scripts that make assumptions about these extensions could take a little bit of coordinated effort, so ti might take a few days to get organized enough for the PR to be merged, but I am willing to spend some time making sure everything will work out 😄

@kotp
Copy link
Member

kotp commented Dec 29, 2019

If the exercise files are executable, and not considered library files, then I agree with removing the extension. The "magic comment" indicates the file type, and the file should be delivered with execute bit set. This is where we are going as far as providing "skeleton" files for the exercises, now, right?

@guygastineau
Copy link
Contributor

Yes, @kotp you are right. I don't think they are all executable currently, but we can change that.

@kotp
Copy link
Member

kotp commented Dec 30, 2019

Do we want to encourage them as executable scripts, or more as libraries?

@guygastineau
Copy link
Contributor

@kotp still a great question. It seems like we had reached consensus that we should create a library style for the tests. None of the work has happened for that.

I also remember something coming out of the how did you learn Bashj? issue that suggested our exercises appear contrived (read not very shelly) to some users. If our goal is to make the experience feel as much like the general learning process for shell (scripts to get stuff done like command line heroes), then creating a sort of synthetic, academic buffer between the work and the tests is likely to undermine that goal. So, I also see a benefit here in abandoning the library idea.

Really, I think we need to see the way forward for exercism V3 more clearly before we can know how to handle this. The more time I spend thinking about these somewhat stale issues preparing for the new year the more I realize we practically need to get all of that stuff sorted out before we can move forward with much without having to redo a lot of work.

Is your head in the same place mine is about this?

@kotp
Copy link
Member

kotp commented Dec 30, 2019

I think it is. Bash is such a different beast because of how it is used than other, even scripting, languages.

@braoult
Copy link
Contributor

braoult commented Sep 15, 2021

(From Slack discussion): I can do it, if everybody agrees: change shebang from bash to bats, and extension from .sh to .bats. I will look around too if some files refer to xxx_test.sh.

I am less clear on where are server-side tests (they will need to accept the 2 extensions).

Should solutions extension also be changed ? If yes, to .bash or dropped ? I believe some editors may not understand shebang, so .bash may be more secure than removing extension (for auto indent/completion/colors/syntax checkers, etc...). I don't know. @koalaman made some stats on shell scripts on github, if this can help.

About shellcheck mention above, bats files are supported for some time now (given proper shebang/extension). But the wrong shebang makes Emacs' flycheck fail: By default it uses shellcheck (OK) and bash -n (NOK) to check syntax of bash scripts. This is only the reason why I raised this subject on Slack :-)

@braoult
Copy link
Contributor

braoult commented Sep 17, 2021

See exercism/bash-test-runner#27 and #541.

@glennj glennj linked a pull request Sep 17, 2021 that will close this issue
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.

6 participants