Skip to content

Conversation

@elubin
Copy link

@elubin elubin commented Sep 18, 2013

No description provided.

Choose a reason for hiding this comment

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

There are some speech tags that have more than 1 speaker. I made this mistake initially too. :)

Copy link
Author

Choose a reason for hiding this comment

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

can you provide an example?

Choose a reason for hiding this comment

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

Line number: 1738 of macbeth.xml

<SPEECH>
<SPEAKER>MACBETH</SPEAKER>
<SPEAKER>LENNOX</SPEAKER>
<LINE>What's the matter.</LINE>
</SPEECH>

Copy link
Author

Choose a reason for hiding this comment

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

ugh. we need a DTD for this file to fail against ;)

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Oops, I missed this too

@JESii
Copy link

JESii commented Sep 19, 2013

I know when using rspec, the common approach is to put all your test files in a separate spec directory; don't know if this is the case with MiniTest, but doing that would make it clearer where things are. I thought at first you didn't have any tests :=)

@elubin
Copy link
Author

elubin commented Sep 19, 2013

agreed, i should probably move the tests to a directory. there isn't too much documentation on minitest, but i never used it before so wanted to try it. testing the 'puts' method was very tricky!! :)

@elubin
Copy link
Author

elubin commented Sep 19, 2013

"I like the way you organized your tests with a separate describe for each group of related tests." thanks. that's the way i was taught and it reads well when you run verbose. also, if you name the describe with the method name like describe '#parse' it's VERY easy to find the bug.

@JESii
Copy link

JESii commented Sep 19, 2013

I know what you mean; I really like the fact that MiniTest automatically includes the capture_io method!

Jon Seidel, CMC®

Effective Decisions... Priceless!

From: elubin [mailto:[email protected]]
Sent: Thursday, September 19, 2013 6:35 AM
To: thoughtbot/shakespeare_analyzer
Cc: Jon Seidel
Subject: Re: [shakespeare_analyzer] Macbeth Analyzer Solution (#8)

agreed, i should probably move the tests to a directory. there isn't too much documentation on minitest, but i never used it before so wanted to try it. testing the 'puts' method was very tricky!! :)


Reply to this email directly or view it on GitHub #8 (comment) . https://github.com/notifications/beacon/R_jmgpfh-Dl35zqLlFcAJmO5uC46nraTs5-T6N_e6sO6Cg3TlMU8QTPnRhxvORYA.gif

Copy link

Choose a reason for hiding this comment

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

A multiline string would probably be more readable here, e.g. by using """.

@r00k
Copy link
Contributor

r00k commented Sep 26, 2013

Good effort overall!

I think the biggest thing you could do to improve this is to treat your test code as equally important as your production code.

Your production code is clean, well-organized, and easy to read. Your tests don't quite achieve the same level of clarity.

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.

6 participants