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

reworked pangram spec#406

Closed
Vankog wants to merge 1 commit intoexercism:masterfrom
Vankog:rework-pangram-spec
Closed

reworked pangram spec#406
Vankog wants to merge 1 commit intoexercism:masterfrom
Vankog:rework-pangram-spec

Conversation

@Vankog
Copy link
Copy Markdown
Contributor

@Vankog Vankog commented Sep 6, 2017

The spec file had some strange tests that are not really worth teaching to new devs.
I think we should really have an eye on proper test conventions when reviewing.
(this also is the case for other exercises, but I've only done this for now)

What do you think about this version?
Better? worse? Something I could change?
I think my proposal has a nicer test progression from a TDD point of view and removes unnecessarily duplicated cases and utilizes jasmine's BDD-oriented test description syntax (I hope).

unique;

var Pangram = function(candidate) {
var msg = candidate || '';
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.

had to add a missing null check


xit('pangram with non-ascii characters', function() {
var pangram = new Pangram("Victor jagt zwölf Boxkämpfer quer über den großen Sylter Deich.");
xit('ignores non-ANSI characters', function() {
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.

Trying to bring Unicode to devs attention.

@Vankog Vankog changed the title rework pangram spec reworked pangram spec Sep 6, 2017
@matthewmorgan
Copy link
Copy Markdown
Contributor

@Vankog thanks for taking this on. In general my feeling on these changes is that the tests should be brought in to line with the latest canonical data. Usually the only time we want to vary from that is if there is some JavaScript-specific issue or behavior that ought to be called out. A good example would be null vs undefined vs an empty string. I haven't commented on specific changes, because it looks like this specfile diverges from the canonical one, both in its current form and in your proposed changes.

So, I think the best thing would be:

  1. The tests should be updated to match the canonical spec, with the caveat that it's OK and in fact desirable to highlight quirks of this language.
  2. If you feel that a test should be added, changed, or removed from the spec, you should make that case in an issue on https://github.com/exercism/problem-specifications. Once there's consensus there, a PR could be made with the changes. At that point it would make sense to bring those changes to specific tracks.

Does that make sense to you?

@Vankog
Copy link
Copy Markdown
Contributor Author

Vankog commented Sep 6, 2017

OK, I didn't have the feeling that the canonical tests were taken quite seriously in the tracks. Especially, because the canonicals left some things open to desire. So I took them for inspiration and made sure that they are indeed covered by my new spec. But I also expanded the cases to important edge cases.
So you mean that unless it's something language specific we should really stick only to the canonical cases, without further extension? That feels strange to me, but I see your reasoning to change the canonical definitions first to keep it in sync.

@matthewmorgan
Copy link
Copy Markdown
Contributor

@Vankog actually, what I'm saying is: if you feel strongly about the need to add, remove, or change tests, they should first be added, removed, or changed in the canonical specfile, then in each language track.

I'm not married to the current set of tests, which don't match the canonical specfile, so I would be in favor of changing them, but the right way to do it is by first getting some community consensus on what the right set of tests is.

Does that make sense?

In general I feel the right set of tests not only covers all the cases, but helps the developer recognize the desired input/output patterns. A map, without dictating internal implementation details, to a working solution.

I think you've identified some weaknesses in the track that also exist in the canonical specfile.

@Vankog
Copy link
Copy Markdown
Contributor Author

Vankog commented Sep 8, 2017

OK, as far as I'm concerned I understood you correctly.
I'll file an issue in the canonical track for discussion.

@Vankog
Copy link
Copy Markdown
Contributor Author

Vankog commented Sep 8, 2017

OK, here is the discussion in the canonical track:
exercism/problem-specifications#893

@stale
Copy link
Copy Markdown

stale Bot commented Nov 16, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the wontfix label Nov 16, 2017
@Vankog
Copy link
Copy Markdown
Contributor Author

Vankog commented Nov 20, 2017

*i'll open it again when the time is ready (just came back from a vacation anyway).

@Vankog Vankog closed this Nov 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants