exercism / ruby

Exercism exercises in Ruby.
https://exercism.org/tracks/ruby
MIT License
548 stars 517 forks source link

Remove the shebang? #295

Closed kytrinyx closed 7 years ago

kytrinyx commented 8 years ago

Should we revisit the decision to add a #! line to the test suites?

Here's the most recent discussion about it: https://github.com/exercism/xruby/pull/289 Here's the original discussion: https://github.com/exercism/xruby/issues/93

I was pretty neutral on this, but have come down on the "I'd rather not have it". It adds noise, it's not very conventional, and there are great tools that people should be introduced to if this is their jam.

kotp commented 8 years ago

My quick thoughts about it, both for and against...

kotp commented 8 years ago

The shebang being there doesn't get in the way... if the nitpicker chooses to make the file executable, they are allowed. It makes sense that tests are runnable, we don't change the execution bit on their machine, but the shebang makes it simple to simply do so if they wish to. It doesn't get in the way, it is conventional on Unix to provide magic strings, and it communicates that it "could" be made executable and the file system will simply know.

On Windows, this is done in another way, by adding the .rb to the filename or .wrb if you want it to execute in a 'window'. And we already give Windows their "magic" string by default.

remcopeereboom commented 8 years ago

I'm not sure if I should be part of this discussion, but I noticed it under the open issues.

I don't actually ever run using the shebang due to how I have my editor set-up (and the fact that I frequently run under windows), but it certainly has never bothered me. I've also never seen any exercism users complain about it. Have there been concrete problems that have led to the opening of this issue?

there are great tools that people should be introduced to if this is their jam.

I agree that it would be nice to introduce people to some tools, but I think you should be hesitant about forcing them on your users. Using tools might confuse people and make exercism harder to use; even if the tools are considered part of the standard toolchain. From the feedback I've gotten from the site, I can tell you that the majority of users are completely unaware that such tools exist. Most have never even used rake or heard of things like rubocop.

kytrinyx commented 8 years ago

I'm not sure if I should be part of this discussion

Yes, please!

I agree that it would be nice to introduce people to some tools, but I think you should be hesitant about forcing them on your users.

Agreed, this should be optional.

That said, I have never, ever seen projects where they stick a shebang in the test suite, and I don't feel like it teaches people any sort of good practice.

kotp commented 8 years ago

What you said... the shebang in "projects' test suites", but that is because usually projects have test tools.

We don't present projects for them. We have an exercise where they are expected to run that one test file. Heck, they aren't even expected to run the code they are writing. This isn't "that" thing.

We aren't teaching projects.

Because the _test.rb file is the executable here, it is appropriate, perhaps even doubly relevant. But we don't even 'activate' it for the nix's, the 'dows' and 'dos' machines get it for free because of the extension, though some dos environments they still need to 'activate' it.

I don't feel strongly about it, actually. So that isn't the tone here. But the comparison isn't the same for the tests they get from the exercism client and a project environment.

Inside here, that is a little different. We don't need the shebang at all, we have make test-assignment while we are development, and the git hook, and perhaps another tool or two in bin for the actual project.

The exercismist may simply chmod the test and invoke it without even calling "ruby" on it with that magic string. The Windows folks get that "magic" automatically due to the extension, not sure why we don't for the Nix's, since the goal is to run "that one file" that happens to be a _test.rb that has tests, but which is the "focus" of that tool.

Still, I don't feel strongly for or against it, it is easy enough for them to learn how to create a shebang that makes sense for them. We actually have ours in a "preferred way" which is a good example of that specific thing. There are other ways to do the same thing that have subtle problems. At least we do it right, it stays out of the way, it is there if they want it (no one needs it, really).

If we don't have it, again, no problem. The Windows folks keep it, the Linux/BSD/Unix/etc, lose it.

kytrinyx commented 8 years ago

The exercismist may simply chmod the test and invoked it without even calling "ruby" on it with that magic string.

I still don't see the benefit of calling it without ruby. I call my tests with ruby all the time. Then I bind it to a key so that I don't have to type it over and over again. Or I use the arrow up to find it in history. Or control-r to find it farther back in history.

I get that an exercise isn't a project, but have you ever seen anyone put shebangs in their ruby tests?

I think what I'm trying to get at is that we are adding a line of code that I think it would be a bad idea to emulate elsewhere.

stale[bot] commented 7 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

hilary commented 7 years ago

Closing this issue as the last meaningful activity was over a year ago.

kytrinyx commented 7 years ago

This has come up again, see https://github.com/JacobNinja/exercism-analysis/issues/15 and https://github.com/exercism/rikki-ruby-analyzer/pull/6

I would prefer to remove the shebangs, but this is not my track and I would 100% defer to your choice in this matter.

budmc29 commented 7 years ago

Since every time you commit your test files you will get the shebang message from Rikki which is out of place on the context, I think we definitely need a solution for that. That's just too much noise and possibly inconvenience (for me at least).

There are two propositions I heard:

  1. Remove the shebang from all the ruby test files. This will of course disturb the users that like to run their tests with ./test_name.rb. My thoughts are that having a shebang in the test file forced on you is not really good since it's not necessarily a recommended best practice. I find it tedious to have do chmod +x the test file anyway, so ruby file_name.rb it is for me. That's easily doable, and I volunteer for the search and replace.

  2. Keep the shebang for all the tests but remove the Rikki check. In case no 1 fails, this is a good alternative. You can see my PR here: https://github.com/exercism/rikki-ruby-analyzer/pull/6

Curious to hear your thoughts about this.

kotp commented 7 years ago

I don't think option 2 is an option, because the check is meant to suggest best practices.

I would support removing them from the test files and generators themselves. This will involve:

As you go through the generator, there will be different issues that you come across. Some will work flawlessly, others, like Dominoes will snag. It would be good to report them so we can address the things that don't work well.

I believe there are other exercises that aren't generated yet, like Food Chain and Proverb. Those will be just a removal of the shebang.

But it is not just the test files, the files in bin/ folder should be looked at, which tie into the local git hook for pre-push. And the bin/local-status-check also runs the bin/executable-tests-check which is related.

Insti commented 7 years ago

Removing the line from the test files is a good idea, let's do it.

This will involve:

  • test/fixtures/xruby/lib/generator/test_template.erb
    • test/generator/implementation_test.rb

These are the LEAST important files to change.

Todo:

(Merge #684 which fixes dominoes. (@kotp))

Remove the shebang line from: lib/generator/test_template.erb Run bin/generate --all to regenerate the tests. Go through and remove the line from the remaining exercise/*/*_test.rb files.

kytrinyx commented 7 years ago

Alrighty, it sounds like the consensus is to remove the shebang. I'll close the rikki-related pull requests.

kotp commented 7 years ago

Looks like the todo list:

The following files are not generated and so will need to have the shebang removed manually.:

Non-generated filename checklist - [ ] `exercises/binary-search/binary_search_test.rb` - [ ] `exercises/proverb/proverb_test.rb` - [ ] `exercises/flatten-array/flatten_array_test.rb` - [ ] `exercises/meetup/meetup_test.rb` - [ ] `exercises/twelve-days/twelve_days_test.rb` - [ ] `exercises/grade-school/grade_school_test.rb` - [ ] `exercises/hexadecimal/hexadecimal_test.rb` - [ ] `exercises/kindergarten-garden/kindergarten_garden_test.rb` - [ ] `exercises/atbash-cipher/atbash_cipher_test.rb` - [ ] `exercises/saddle-points/saddle_points_test.rb` - [ ] `exercises/series/series_test.rb` - [ ] `exercises/accumulate/accumulate_test.rb` - [ ] `exercises/nucleotide-count/nucleotide_count_test.rb` - [ ] `exercises/crypto-square/crypto_square_test.rb` - [ ] `exercises/minesweeper/minesweeper_test.rb` - [ ] `exercises/diamond/diamond_test.rb` - [ ] `exercises/pythagorean-triplet/pythagorean_triplet_test.rb` - [ ] `exercises/bowling/bowling_test.rb` - [ ] `exercises/scrabble-score/scrabble_score_test.rb` - [ ] `exercises/house/house_test.rb` - [ ] `exercises/palindrome-products/palindrome_products_test.rb` - [ ] `exercises/space-age/space_age_test.rb` - [ ] `exercises/secret-handshake/secret_handshake_test.rb` - [ ] `exercises/poker/poker_test.rb` - [ ] `exercises/octal/octal_test.rb` - [ ] `exercises/food-chain/food_chain_test.rb` - [ ] `exercises/simple-cipher/simple_cipher_test.rb` - [ ] `exercises/trinary/trinary_test.rb` - [ ] `exercises/robot-simulator/robot_simulator_test.rb` - [ ] `exercises/robot-name/robot_name_test.rb` - [ ] `exercises/protein-translation/protein_translation_test.rb` - [ ] `exercises/linked-list/linked_list_test.rb` - [ ] `exercises/scale-generator/scale_generator_test.rb` - [ ] `exercises/connect/connect_test.rb` - [ ] `exercises/allergies/allergies_test.rb` - [ ] `exercises/alphametics/alphametics_test.rb` - [ ] `exercises/point-mutations/point_mutations_test.rb` - [ ] `exercises/prime-factors/prime_factors_test.rb` - [ ] `exercises/binary-search-tree/binary_search_tree_test.rb` - [ ] `exercises/strain/strain_test.rb` - [ ] `exercises/circular-buffer/circular_buffer_test.rb` - [ ] `exercises/leap/leap_test.rb` - [ ] `exercises/dominoes/dominoes_test.rb` - [ ] `exercises/matrix/matrix_test.rb` - [ ] `exercises/hello-world/hello_world_test.rb` - [ ] `exercises/pascals-triangle/pascals_triangle_test.rb`


Hope the listing helps.

Anything I missed this time around @insti?

budmc29 commented 7 years ago

Awesome @kotp, the todo list is a great help, will get started on the task today.

kotp commented 7 years ago

Closed via #685

kotp commented 7 years ago

Thank you everyone for the discussion... and thank you @budmc29 for taking this task full on.