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

Bash linting #116

Closed
sjwarner-bp opened this issue Dec 29, 2017 · 17 comments
Closed

Bash linting #116

sjwarner-bp opened this issue Dec 29, 2017 · 17 comments
Labels

Comments

@sjwarner-bp
Copy link
Contributor

I've noticed that there isn't linting as part of the track yet. In my experience contributing to the Python track I see they have implemented linting, and it allows a greater track consistency.

I'd be interested in hearing other's thoughts around linting of code (and if anyone here has any particular favourites when it comes to linters for Bash).

A cursory glance points me towards shellcheck, which seems to integrate with Travis alright!

@budmc29
Copy link
Member

budmc29 commented Dec 30, 2017

That looks good and definitely something that will help, I tested it with a sample script and the suggestions were good.

One downside might be with Bats support, as per: koalaman/shellcheck#417, which is not yet merged.

@sjwarner-bp
Copy link
Contributor Author

sjwarner-bp commented Dec 30, 2017

That's a great point - I didn't see this before.

If we were set on using shellcheck as a linter, we could omit testing the bats files. Equally, it seems as though there is a good PR attached to the issue, so hopefully it may be merged in the future (or just use the unmerged branch - people seem to think it works pretty well).

Conversely, I have no particular attachment to shellcheck, and as such am happy to take suggestions for any other linters 🙂

@kotp
Copy link
Member

kotp commented Jan 1, 2018

Linting on the bats file is the test file, and the test file gets seen by the user/learner, and therefore should be as correct as can be.

The solutions are submitted by contributors, and it would be nice to conform to conventions, but this has somewhat less exposure to a learner.

@budmc29
Copy link
Member

budmc29 commented Jan 2, 2018

I can give this a try and see how it goes after #104 is completed, since it's dependent on that.

@budmc29 budmc29 self-assigned this Jan 2, 2018
@sjwarner-bp
Copy link
Contributor Author

sjwarner-bp commented Feb 13, 2018

Any progress on this one @budmc29 ? 🙂

I imagine there will be a lot of edits with this.

@budmc29
Copy link
Member

budmc29 commented Feb 14, 2018

Forgot about this, I'll see what I can do in the following days.

@budmc29 budmc29 removed their assignment Jul 16, 2018
@guygastineau
Copy link
Contributor

Did #148 take care of this?

@budmc29
Copy link
Member

budmc29 commented Jul 24, 2018

No, this is a different issue.
I don't have time to work on it anymore, so if you can take over if you want.

@guygastineau
Copy link
Contributor

guygastineau commented Jul 24, 2018

I'll consider this. I'm going to focus on updating the out of date tests with the generator first, so we can get version files for everything.

@guygastineau
Copy link
Contributor

I'll check later today, but I believe shellcheck gets mad if you use [ ] instead of [[ ]].

I think if we implement it we will have to change all of our tests to use [[ ]].

@budmc29
Copy link
Member

budmc29 commented Jul 26, 2018

I see no issue with that, it's actually an improvement if my understand of how [[ works is correct.

@guygastineau
Copy link
Contributor

Okay, great. I will look into adding linting with shellcheck.

I guess we will need to make other scripts and bin/* follow the same guidelines.

@guygastineau
Copy link
Contributor

I will look into this again after I have gotten a couple more exercises ported to the track.

@guygastineau
Copy link
Contributor

A glance through our example solutions suggests that most of them would need changing before they would pass linting with shellcheck. Some of them will need outright replacement

@budmc29 budmc29 self-assigned this Aug 19, 2018
@budmc29
Copy link
Member

budmc29 commented Aug 19, 2018

I'll have another try at this.

@budmc29
Copy link
Member

budmc29 commented Oct 24, 2018

I have closed my attempt since I don't have that much time anymore, but if anyone is interested you can see the progress I made on #234

@budmc29
Copy link
Member

budmc29 commented Jan 15, 2019

I'll close this. If anyone has time to pick it up, feel free to reopen.

@budmc29 budmc29 closed this as completed Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants