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

Add new exercise grep #752

Merged
merged 7 commits into from
Jan 15, 2020
Merged

Conversation

TomPradat
Copy link
Contributor

Fixes #737 .

I was wondering about the fact that this exercise is a CLI.

Unless I missed something, it's not possible to test a file that just executes a set of instructions.

I was thinking about exporting a function that takes the pattern, the files and options as arguments like that :

export default (pattern, files, options =  {}) => {}

Is the user supposed to use the filesystem or should I provide him the data in the files in some way ?

@SleeplessByte
Copy link
Member

Great question. From the canonical data:

    " The language track implementing this exercise       ",
    " should ensure that when the tests run, three files  ",
    " are created with the following contents. The file   ",
    " names and their contents are listed below:          ",

I think we should actually create these files, instead of having them be generated by the tests. So, add iliad.txt, midsummer-night.txt and paradise-lost.txt as files to the exercise directory grep (these will be downloaded automagically when using the exercism cli!)

For your second question, let's see if @tejasbubane agrees with this, I actually think it would be great if you make this script executable. That means, NO EXPORTS. I think we should do spawn or spawnSync and execute node on the file, with actual arguments, and use stdout for parsing. This exercise a the perfect opportunity to learn about:

  • process.args / process.argv0
  • process.stdout / process.stderr
  • path
  • fs

Example:

describe("tests something", () => {
  it("works", () => {
    const output = spawnSync('node', ['grep.js', '"Agamemnon"', 'iliad.txt'], { stdio: 'pipe' }).stdout.toString().trim()
    expect(output).toBe("Of Atreus, Agamemnon, King of men.")
  })
})

Stub

Add the hashbang at the top so people can actually execute it on UNIX/POSIX environments:

#!/usr/bin/env node

We can even debate adding a "cross env" wrapper grep.sh and grep.bat, but that is to be seen.

@TomPradat
Copy link
Contributor Author

Alright, I didn't know about spawSync, I agree this is a great opportunity to learn more about nodejs, I'll go on this 👍

@TomPradat
Copy link
Contributor Author

The MVP is working, i'll finish this ASAP :)

@TomPradat
Copy link
Contributor Author

So i got a problem because the test script only copies the example.js and the spec file, which causes the script to fail because the .txt files are not copied

@SleeplessByte
Copy link
Member

So i got a problem because the test script only copies the example.js and the spec file, which causes the script to fail because the .txt files are not copied

Yes! Let's add them.

If you look at this code: https://github.com/exercism/javascript/blob/master/scripts/helpers.js#L74-L77

  const libDir = ['exercises', assignment, 'lib'].join('/');
  if(shell.test('-d', libDir)) {
    shell.cp(libDir + '/*.js', 'tmp_exercises/lib');
  }

... you can see the lib dir is being copied, as long as it ends on .js. Let's add a data folder, where we copy anything:

  const dataDir = ['exercises', assignment, 'data'].join('/');
  if(shell.test('-d', dataDir)) {
    shell.cp([dataDir, '*'].join('/'), 'tmp_exercises/data');
  }

@TomPradat
Copy link
Contributor Author

So i got a problem because the test script only copies the example.js and the spec file, which causes the script to fail because the .txt files are not copied

Yes! Let's add them.

If you look at this code: https://github.com/exercism/javascript/blob/master/scripts/helpers.js#L74-L77

  const libDir = ['exercises', assignment, 'lib'].join('/');
  if(shell.test('-d', libDir)) {
    shell.cp(libDir + '/*.js', 'tmp_exercises/lib');
  }

... you can see the lib dir is being copied, as long as it ends on .js. Let's add a data folder, where we copy anything:

  const dataDir = ['exercises', assignment, 'data'].join('/');
  if(shell.test('-d', dataDir)) {
    shell.cp([dataDir, '*'].join('/'), 'tmp_exercises/data');
  }

Nice, should I add it myself ?

@SleeplessByte
Copy link
Member

Yes go ahead!

@TomPradat
Copy link
Contributor Author

Should I add stuff in the stub file ? Or should we let the user do the exercise from scratch ?

Also should I write a README.md ?

@SleeplessByte
Copy link
Member

See CONTRIBUTING for the readme generation. You don't need to do this yourself.

Let's add the stub file, but only have a comment with 'this is a stub blablabla'

@TomPradat TomPradat changed the title WIP : Add new exercise grep Add new exercise grep Oct 26, 2019
@TomPradat
Copy link
Contributor Author

If everything is ok, i can rebase my last 3 commits and we can merge this :)

@SleeplessByte
Copy link
Member

You don't need to rebase. I'll squash-merge

@TomPradat
Copy link
Contributor Author

TomPradat commented Dec 17, 2019

Hi, do you have some time to merge it ?

config.json Outdated Show resolved Hide resolved
config.json Show resolved Hide resolved
exercises/grep/grep.js Show resolved Hide resolved
exercises/grep/grep.spec.js Outdated Show resolved Hide resolved
exercises/grep/grep.spec.js Outdated Show resolved Hide resolved
exercises/grep/grep.spec.js Outdated Show resolved Hide resolved
exercises/grep/grep.spec.js Outdated Show resolved Hide resolved
@TomPradat
Copy link
Contributor Author

This should be good now I think

exercises/grep/grep.js Outdated Show resolved Hide resolved
@Calamari
Copy link
Contributor

I like this exercise, I think it would make a great addition to explore JS more.

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

I absolutely love this exercise. Thank you so much for writing this @TomPradat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new exercise: grep
4 participants