Skip to content

Added FlattenArray generator #473

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

Merged

Conversation

felix91gr
Copy link
Contributor

I have a problem here. The null values in the Canonical Data file for this exercise aren't rendered correctly at the moment of generating the test. In fact, they are ommited:

{
      "description": "all values in nested list are null",
      "property": "flatten",
      "input": [null, [[[null]]], null, null, [[null, null], null], null],
      "expected": []
}

gets rendered as:

public void All_values_in_nested_list_are_null()
{
        Assert.Empty(FlattenArray.Flatten(new[] { , new[] { new[] { new[] {  } } }, , , new[] { new[] { ,  },  },  }));
}

Is there a way to UpdateCanonicalData the nulls into this one?

Assuming there isn't, I digged around in the code base and the only thing I could find that might be amiss is the ValueFormatter class. That's the one in charge of rendering, but that's as far as I can go today.

Pls halp.

@ErikSchierboom
Copy link
Member

Well, the canonical data often uses null to denote a missing value, which is why we don't actually render it. You could see what happens if you add it to the value formatter and re-run all the generators to see which ones break. If too much breaks, I would suggest converting the null values to new EscapedValue("null") instances.

@felix91gr felix91gr force-pushed the adding-flatten-array-generator branch from 8b5e508 to 2adf049 Compare October 11, 2017 19:03
@felix91gr
Copy link
Contributor Author

I would suggest converting the null values to new EscapedValue("null") instances.

You mean UnescapedValue? Like I did in #472?

@felix91gr
Copy link
Contributor Author

Okay so, I tried this...

You could see what happens if you add it to the value formatter and re-run all the generators to see which ones break

And I only broke 1 test on 1 exercise. That's nice. The bad thing is, now this test doesn't compile XD

I'll investigate. Counsel is very much appreciated, because I'm only now understanding the Exercise codebase.

@felix91gr
Copy link
Contributor Author

An alternative could be to use List<T>, maybe... 🤔 That's how the original test is written in master:

var nestedList = new List<object>
{
    null,
    new List<object>
    {
        null,
        new List<object> { null },
        new List<object> { new List<object> { new List<object> { null } } }
    },
    null
};

@felix91gr
Copy link
Contributor Author

Now that I look closely at the two-bucket exercise, it doesn't appear to have been broken by this commit. It includes absolutely no nulls whatsoever.

Was it broken on another commit?

@ErikSchierboom
Copy link
Member

@felix91gr I wouldn't worry about the two-bucket exercise, it is most likely due to some change in the canonical data. Just make sure you don't add that file to this PR.

I'm also perfectly fine with using List<object>, you can choose what you like.

@felix91gr felix91gr force-pushed the adding-flatten-array-generator branch from b470def to 6760366 Compare October 12, 2017 20:16
@felix91gr
Copy link
Contributor Author

Okay, I reverted all the tests I generated in that run, except for the flatten-array one.

The problem seems to be that the type checker can't figure out a type for the arrays I'm giving it. That's why I suggested that it should probably be a List.

On the other hand... I just tested changing the arrays by hand from: new[] to new object[] and it actually type-checks and passes the tests!

Do you know how I can do this from the generator? @ErikSchierboom

@ErikSchierboom
Copy link
Member

Hmmm, I don't think there is an easy way to do that at the moment. I think you'll have to manually create the rendered string.

@felix91gr
Copy link
Contributor Author

felix91gr commented Oct 12, 2017 via email

@ErikSchierboom
Copy link
Member

@felix91gr For templating we use dotliquid. However, as you want to do a recursive rendering, I think you should just do some string manipulation.

@felix91gr felix91gr force-pushed the adding-flatten-array-generator branch from 6760366 to a463421 Compare October 13, 2017 20:07
@felix91gr
Copy link
Contributor Author

I think I finally understood how UnescapedValues work. I've finished the generator now :)

@felix91gr felix91gr changed the title [WIP] Added FlattenArray generator Added FlattenArray generator Oct 13, 2017
@ErikSchierboom ErikSchierboom merged commit 2fe1dce into exercism:master Oct 13, 2017
@ErikSchierboom
Copy link
Member

Merged. Awesome!

@felix91gr felix91gr deleted the adding-flatten-array-generator branch October 13, 2017 21:59
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.

2 participants