Skip to content

Implement exercise complex-numbers#367

Merged
matthewmorgan merged 1 commit intoexercism:masterfrom
MattH-be:complex-numbers
Oct 31, 2017
Merged

Implement exercise complex-numbers#367
matthewmorgan merged 1 commit intoexercism:masterfrom
MattH-be:complex-numbers

Conversation

@MattH-be
Copy link
Copy Markdown
Contributor

No description provided.

@matthewmorgan
Copy link
Copy Markdown
Contributor

@MattH-be looks like you have a merge conflict in config.json, probably because your last PR added text at the same spot in the file. Would you mind resolving that and pushing up the change?

Copy link
Copy Markdown
Contributor

@matthewmorgan matthewmorgan left a comment

Choose a reason for hiding this comment

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

@MattH-be nice clean implementation here-- looks great! I have one piece of feedback on the test suite structure, and of course the merge conflict needs to be fixed.

Let me know if you have questions or if I can help with either of those things.

@@ -0,0 +1,236 @@
import ComplexNumber from './complex-numbers.js';

xdescribe('Imaginary unit', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We probably should make the describe nesting a little less complicated here. Here's what I'm thinking:

  1. I think it's nice how test blocks are grouped conceptually. But, we don't have any other examples in the entire track with nested describe blocks. Nothing wrong with showing people the possibilities, but that leads me to thought (2):

  2. Sometimes we're using xdescribe to skip an entire block, but once the block is un-skipped, we have multiple unskipped tests, and possibly multiple failures for the user. For this reason, I think it would be better to use xtest, even inside the xdescribe blocks. Let them proceed by passing one test at a time, instead of an entire block.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree. All test blocks but the first one should be skipped with xtest, even if they are nested in a describe block.

About nesting describe blocks, the philosophy of the track is to keep it as simple as possible. By default, there shouldn't be any nested describe. Exercism users just have to make individual tests blocks one by one. I think nesting describes adds noise to that.

Anyway, we can discuss if there is a reason to nest describes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I grouped the tests in decribe blocks as they where grouped in the canonical-data.json file in the problem specifications. I however agree that in order to keep things simple for the users, we should only use one describe block and just skip tests by using xtest.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MattH-be @rchavarria I don't have a problem with the nested describes, but I think the suite looks better now, easier to understand. 👍

@MattH-be
Copy link
Copy Markdown
Contributor Author

All requested changes should've been implemented. Let me know if you need anything else changed!

@matthewmorgan
Copy link
Copy Markdown
Contributor

@MattH-be Looks like we have some merge conflicts still. Perhaps another PR was merged in and caused that.

Can you please:

  1. Resolve the merge conflicts?
  2. Squash to one commit?

Thanks!

Comment thread exercises/complex-numbers/package.json Outdated
"eslint-plugin-import": "^2.2.0",
"eslint-plugin-jsx-a11y": "^5.0.1",
"eslint-plugin-react": "^7.0.1",
"jest": "^20.0.4"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some dependencies (namely jest and babel-jest) were recently upgraded on all exercises here: #368 Can you please update them in this file as well?

@MattH-be
Copy link
Copy Markdown
Contributor Author

Im having some issues with squashing and not running into merge conflicts.

@matthewmorgan
Copy link
Copy Markdown
Contributor

matthewmorgan commented Oct 30, 2017

@MattH-be from what I can see, the merge conflict is happening because the ISBN verifier and complex-numbers blocks in the config.json are adjacent. Adjacent (or same line) changes in different commits give git fits.

It's a little complicated to explain how to do this with a fork branch. There may be more than one way, but I think this approach would work:

git remote add upstream https://github.com/exercism/ecmascript.git
git checkout master
git pull upstream master   //this should bring in any changes in the original repo master branch
git checkout complex-numbers
git rebase master

# I found merge conflicts in *config.json* because the isbn-verifier config block 
# and the complex-numbers block were adjacent.  I found that all I had to do 
# was remove the *===* etc boundaries around the conflicts and save the file, then...


git add .
git rebase --continue

At this point you should be able to force-push to your branch on GitHub:

git push -f

This is a pretty big hassle but also worth knowing how to do. Care to give it a try? If it doesn't work out, I should be able to squash it for you-- lmk.

Grouped all tests under a single describe

Ordered the tests in a more logical way.
All tests except the first one are skipped.

Removed mention of enabling tests by changing xdescribe to describe.

Implement exercise isbn-verifier (exercism#366)

[Housekeeping] Upgrade jest (exercism#368)

* Use `package-lock.json` instead of `yarn.lock`

* Upgrade jest and babel-jest to latest versions

Jest to 21.2.1
babel-jest to 21.2.0

Change Exercise is now ECMAScript ready (exercism#362)

Transpose Exercise is ECMAScript ready (exercism#364)

Upgrade jest

Protein Translation is ECMAScript ready (exercism#365) (exercism#365)

Update  setup links in all READMEs (exercism#371)

Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation

Closes exercism#370

Updated setup link
@MattH-be
Copy link
Copy Markdown
Contributor Author

That fixed it. I was always doing git rebase -i HEAD~number of commits. Glad to know there's a better way of doing it. Was kinda wondering if there was, because it was a really devious way of doing things.

@matthewmorgan matthewmorgan merged commit 33482d6 into exercism:master Oct 31, 2017
@matthewmorgan
Copy link
Copy Markdown
Contributor

Awesome! Glad that worked out for you. Sometimes git makes my head spin...

@MattH-be MattH-be deleted the complex-numbers branch November 2, 2017 12:59
laurmurclar pushed a commit to laurmurclar/ecmascript that referenced this pull request Nov 4, 2017
Grouped all tests under a single describe

Ordered the tests in a more logical way.
All tests except the first one are skipped.

Removed mention of enabling tests by changing xdescribe to describe.

Implement exercise isbn-verifier (exercism#366)

[Housekeeping] Upgrade jest (exercism#368)

* Use `package-lock.json` instead of `yarn.lock`

* Upgrade jest and babel-jest to latest versions

Jest to 21.2.1
babel-jest to 21.2.0

Change Exercise is now ECMAScript ready (exercism#362)

Transpose Exercise is ECMAScript ready (exercism#364)

Upgrade jest

Protein Translation is ECMAScript ready (exercism#365) (exercism#365)

Update  setup links in all READMEs (exercism#371)

Update all setup links in exercise READMEs from http://exercism.io/languages/ecmascript to http://exercism.io/languages/ecmascript/installation

Closes exercism#370

Updated setup link
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.

4 participants