Skip to content

add connect generator and modify class to accept array input #464

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
merged 3 commits into from
Oct 9, 2017

Conversation

bmeverett
Copy link
Contributor

Adds the Connect generator. Minor modifications to the class to accept an array as input.

Resolves #404

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I have two comments, but other than that it looks great!

{
return string.Join("\n", board.Select(x => x.Replace(" ", "")));
var board = new[] { ".....", ".....", ".....", ".....", "....." };
Copy link
Member

Choose a reason for hiding this comment

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

For the Connect case, it is quite important that the input strings are aligned nicely under each other, to allow users to see the pattern that needs to be connected. By default, the generator framework does not support this. but perhaps you can "borrow" from this generator?

canonicalDataCase.SetConstructorInputParameters("board");

//remove whitespace
var trimBoard = new List<string>();
Copy link
Member

Choose a reason for hiding this comment

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

Why trim the white space? The original implementation also doesn't trim it and this makes it look better IMHO. Could you remove the trimming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't trim it the tests where failing because I think it's looping through every character and taking the spaces as an empty space. I could be wrong, I'll look into it.

@bmeverett
Copy link
Contributor Author

@ErikSchierboom I pushed up some changes. I aligned the arrays better, but I couldn't figure out how to get rid of the empty line at the end of the array. Besides that, hopefully I addressed your comments.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

It's much better already! I have a small nit about the indentation. As for how you can get rid of the newline, you could also use some plain old LINQ to format the lines, which would make it a lot easier to control such things. If you don't agree or it doesn't work out, feel free to ignore this and only fix the indentation as requested in the comment.

Thanks a lot for the hard work!

{
return string.Join("\n", board.Select(x => x.Replace(" ", "")));
var board = new []
{
Copy link
Member

Choose a reason for hiding this comment

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

Could you perhaps use less indentation? Something like this looks better IMHO:

var board = new [] 
{ 
    ". . . . .",
    " . . . . .",
    "  . . . . .",
    "   . . . . .",
    "    . . . . ."
};

@ErikSchierboom ErikSchierboom merged commit 5b22854 into exercism:master Oct 9, 2017
@ErikSchierboom
Copy link
Member

Merged. Thanks a lot. 🎉

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