Skip to content
This repository was archived by the owner on Aug 1, 2021. It is now read-only.

New Proposed JS Exercism - bracket-push#31

Closed
ginna-baker wants to merge 61 commits intoexercism:masterfrom
ginna-baker:master
Closed

New Proposed JS Exercism - bracket-push#31
ginna-baker wants to merge 61 commits intoexercism:masterfrom
ginna-baker:master

Conversation

@ginna-baker
Copy link
Copy Markdown
Contributor

Bracket-push encourages strategic problem solving skills, allowing the
user to check through a series of brackets to ensure that they are
closed correctly. This exercise also has real-world applications in
debugging software.

kytrinyx and others added 30 commits February 28, 2014 14:53
Make JS test suite pass again.
Come at it the other way so we don't have to muck around with built-ins.
…and changing match to matches -- better responsibility separation also
Add semicolons and parentheses to be consistent.
Update robot simulator spec with semicolons
@kytrinyx
Copy link
Copy Markdown
Member

Cool, I think this looks interesting. @wilmoore as a javascript person, would you take a look at it in terms of style, idioms, and the API that the test suite is enforcing?

@wilmoore
Copy link
Copy Markdown

Hi @ginna-baker,

Thanks for submitting this. I am knee deep in the middle of a sprint ATM; however, I will do my best to look at this and provide useful feedback tonight.

Thanks.

@ginna-baker
Copy link
Copy Markdown
Contributor Author

Thanks, Wil! I'd love to get your thoughts as time allows.

Best,
Ginna

On Aug 26, 2014, at 12:23 PM, Wil Moore III notifications@github.com wrote:

Hi @ginna-baker,

Thanks for submitting this. I am knee deep in the middle of a sprint ATM; however, I will do my best to look at this and provide useful feedback tonight.

Thanks.


Reply to this email directly or view it on GitHub.

@wilmoore
Copy link
Copy Markdown

Here are a few things that I think will get you going toward an idiomatic implementation:

  • name the implementation file example.js rather than bracket-push.js.
  • Start the file with 'use strict';.
  • Use primitive Boolean literals true and false values rather than strings.
  • Use lots of whitespace not only vertically, but also between operators and values.

Here is an example that incorporates the things mentioned above:
https://github.com/exercism/xjavascript/blob/master/nucleotide-count/example.js

For the tests, you can use a single describe with a bunch of nested it calls. Nothing wrong with multiple describe blocks if needed, but probably not needed here.

Here is an example:
https://github.com/exercism/xjavascript/blob/master/nucleotide-count/nucleotide-count_test.spec.js

@ginna-baker
Copy link
Copy Markdown
Contributor Author

Thank you, Wil! I've modified as indicated:

  • renamed file to example.js
  • started example.js with 'use strict';
  • changed strings to Boolean primitive literals;
  • added vertical and horizontal whitespace
  • condensed tests to a single describe, x-ed out all tests but the first one

I also added 'require' at the top of the test spec, but my testem throws an error at 'require', so I'd ask that you double check me on the usage.

Many thanks for the feedback. I'd love to contribute again -- let me know if anything specific is needed.

@wilmoore
Copy link
Copy Markdown

Many thanks for the feedback.

No problem at all :)

I'd love to contribute again -- let me know if anything specific is needed.

I hope to see you contributing further. We are always in need of cleaning up/refactoring assignments.

BTW, you can run a particular assignment's tests with:

make test-assignment ASSIGNMENT= bracket-push

@wilmoore
Copy link
Copy Markdown

For some reason bracket-push/bracket-push.js is still showing up. You may have to git rm that file then push again.

Renamed and refactored as example.js
@ginna-baker
Copy link
Copy Markdown
Contributor Author

Great! I git-rm'ed bracket-push.js. This should be good to go. Let me know if there's anything else I should take care of.

@wilmoore
Copy link
Copy Markdown

wilmoore commented Sep 7, 2014

Hey @ginna-baker. Thanks for the update. There were some large changes to the repo recently which means you'll need to re-clone the project, create a new branch, and drop the bracket-push stuff in there, commit, then create a new PR.

Everything seems to look good to me, so once you open the PR, I should be able to merge it. I'm going to close this one though since it is pretty broken.

Sorry about that.

@wilmoore wilmoore closed this Sep 7, 2014
@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Sep 7, 2014

@ginna-baker if that's intimidating let me know and I'll get your commit in for you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants