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

grep: New exercise #948

Merged
merged 2 commits into from
Jun 6, 2019
Merged

grep: New exercise #948

merged 2 commits into from
Jun 6, 2019

Conversation

pgaspar
Copy link
Member

@pgaspar pgaspar commented Mar 25, 2019

Closes #946.

Implementation

The implementation was heavily influenced by python's. It can certainly be improved, but we may want to leave that for the mentor notes themselves, since the main goal here is to get CI to pass.

Test Generation

The tests are generated using the canonical data, but the file contents are hardcoded in a custom test template. I ended up doing this because I couldn't easily pass custom variables from the test case file to the ERB test template. Without that I can't parse the canonical data comments and make the file contents dynamic, like some tracks do.

I couldn't find other exercises that did anything close to this and looking at the generator code I didn't find a clear way to pass a file_data hash to the template file.

Note: I really want to merge #929... Having the generator write <<-EXPECTED.gsub(/^ */, '').strip instead of <<~EXPECTED on the test file adds a layer of extra complexity we could easily avoid. We've merged this and I updated the code 👍.

@exercism/ruby

TODO

  • Add to config.json
  • Add issue to create mentor notes

@pgaspar pgaspar self-assigned this Mar 25, 2019
- The files are hardcoded in the test template because I couldn't easily
pass new variables from the test case to the ERB test template
- Solution based on the solution on the Python track
Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of it looks 'generally' good to me.

exercises/grep/README.md Show resolved Hide resolved
exercises/grep/README.md Show resolved Hide resolved
@kotp
Copy link
Member

kotp commented May 17, 2019

I noticed the conflglet failure as well as reported by Travis CI. I may have some time to look at the file name problem indicated as well as the configlet issue, if someone doesn't beat me to it, or already have some ideas.

@pgaspar
Copy link
Member Author

pgaspar commented May 23, 2019

I noticed the conflglet failure as well as reported by Travis CI.

Yes, I noted above that before merging we need to add this exercise to config.json. I didn't do that because I wasn't sure where @F3PiX wanted it.

I may have some time to look at the file name problem indicated as well as the configlet issue, if someone doesn't beat me to it, or already have some ideas.

Not sure what file name problem you're referring to 🤔

@kotp
Copy link
Member

kotp commented May 24, 2019

I may have some time to look at the file name problem indicated as well as the configlet issue, if someone doesn't beat me to it, or already have some ideas.

Not sure what file name problem you're referring to 🤔

I think it was corrected... so that part is good to go.

@emcoding
Copy link
Contributor

emcoding commented May 25, 2019

we need to add this exercise to config.json. I didn't do that because I wasn't sure where @F3PiX wanted it.

It should be somewhere at the end of the track. The file reading is a feature above 'level 3'. And moreover, it's also not the simplest form of reading a file. And it's also totally new.

I'd say: let's make it "core": false, "unlocked_by": "12-days"` and difficulty: 8. (Difficulty and topics are not yet part of the Track Anatomy set up, so I don't have opinions on those. For the difficulty I tend to use 2 difficulties for each level but that's not official.)

So, it starts as a side, so the mentors can play with it first, and we can come up with mentor notes.
When we know more know how difficult it really is for students, we can add it to the cores.

Looking sooooooooo forward to trying this :-))))

But I beseech your grace that I may know
The worst that may befall me in this case,
EXPECTED

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking TDD, would it make sense to put all the no-flags tests at the beginning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is defined by the canonical data. Do we want to go against that? (It may be worth considering opening a PR on the problem-specifications repo if you feel strongly about changing the order everywhere).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, no, we don't want to go against that. And as the TDD sequence may differ from language to language, I don't think it's worth to change it in the prob-specs.

@pgaspar pgaspar requested review from kotp and emcoding June 4, 2019 22:11
Copy link
Contributor

@emcoding emcoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks marvellous :-)
I'll leave the technical details to Victor to review.
Thanks so much @pgaspar, for yet another exercise 🎉

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good enough. We can always make improvements and changes later.

Let's get it in! ;)

exercises/grep/README.md Show resolved Hide resolved
@pgaspar
Copy link
Member Author

pgaspar commented Jun 6, 2019

Thank you both!

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 this pull request may close these issues.

New advanced exercise: Add Grep
3 participants