Skip to content

Transpose#364

Merged
rchavarria merged 1 commit intoexercism:masterfrom
RobinCsl:transpose
Oct 29, 2017
Merged

Transpose#364
rchavarria merged 1 commit intoexercism:masterfrom
RobinCsl:transpose

Conversation

@RobinCsl
Copy link
Copy Markdown
Contributor

There is a commit comment relating to files which should have been in their own branch, but I worked on master first, and then realised that to make a second PR, I should have used a separate branch... Hopefully that's not a problem!

@RobinCsl RobinCsl mentioned this pull request Oct 24, 2017
@matthewmorgan
Copy link
Copy Markdown
Contributor

@RobinCsl no worries at all about working on master-- since you're on your own fork, it's not a problem. I'll review.

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.

@RobinCsl if you could please change the language I mentioned in the README, and have a look at my comments on the code suggestions. Code suggestions are entirely optional/food for thought. Let me know how you want to proceed.

Comment thread exercises/transpose/README.md Outdated
$ npm test
```

In many test suites all but the first test have been skipped.
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.

Same language issue I mentioned on your other PR. All tests have been skipped except the first, so might as well say so.

Comment thread exercises/transpose/example.js Outdated
export default function transpose(text) {
const result = [];
text.forEach((line, lineNo) => {
line.split('').forEach((value, key) => {
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.

   [...line].forEach((value, key) => {

The ... is the "spread" operator. It's awesome.

});
});
return result;
}
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.

This all looks good, but it did make me wonder if it's possible to do this entirely functionally-- IE, without having an initial result array that gets mutated. Not critical, but just thinking out loud...

@RobinCsl
Copy link
Copy Markdown
Contributor Author

I updated the README and used the spread operator (it's really cool indeed!). I tried briefly to think about rewriting the example in a purely functional way, but to no avail at the moment.

@matthewmorgan
Copy link
Copy Markdown
Contributor

Looks fine to me! Mind squashing the commits?

@RobinCsl
Copy link
Copy Markdown
Contributor Author

It seems I somehow managed on this one. I'll try to replicate the steps. (But there might be other conflicts since each exercise implementation modifies config.json)

@rchavarria rchavarria merged commit 9297481 into exercism:master Oct 29, 2017
@rchavarria
Copy link
Copy Markdown
Contributor

@matthewmorgan approved changes, commits squased, no conflicts,... merging.

MattH-be added a commit to MattH-be/ecmascript that referenced this pull request Oct 30, 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
@shybyte
Copy link
Copy Markdown

shybyte commented Oct 31, 2017

@RobinCsl The expected test result in the last test ("test many lines") is different from the expected test result in https://github.com/exercism/problem-specifications/blob/master/exercises/transpose/canonical-data.json

So either the canonical test-data is wrong or this PR is wrong.

MattH-be added a commit to MattH-be/ecmascript that referenced this pull request Oct 31, 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
matthewmorgan pushed a commit that referenced this pull request Oct 31, 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 (#366)

[Housekeeping] Upgrade jest (#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 (#362)

Transpose Exercise is ECMAScript ready (#364)

Upgrade jest

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

Update  setup links in all READMEs (#371)

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

Closes #370

Updated setup link
@matthewmorgan
Copy link
Copy Markdown
Contributor

@shybyte good catch! @RobinCsl I need to remove this exercise for the time being. Please have a look and let me know if I can help resolving the issue.

@RobinCsl
Copy link
Copy Markdown
Contributor Author

I had a look for the last hour. I had copy-pasted the code from the old repository and converted it to ES6, but you're right @shybyte, there is an issue with the algorithm. I will work more on that.

@shybyte
Copy link
Copy Markdown

shybyte commented Nov 1, 2017

@RobinCsl Maybe my code for solving the canonical testcases is helpful:

function trimTrailingUndefined(array) {
  const trailingUndefinedCount = [...array].reverse().findIndex(x => x !== undefined);
  return array.slice(0, array.length - trailingUndefinedCount);
}

export default function transpose(input) {
  const maxCol = Math.max(0, ...(input.map(row => row.length)));
  return [...Array(maxCol).keys()].map(col =>
    trimTrailingUndefined(input.map((_v, row) => input[row][col]))
      .map(charOrUndefined => charOrUndefined || ' ')
      .join('')
  );
}

laurmurclar pushed a commit to laurmurclar/ecmascript that referenced this pull request Nov 4, 2017
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