Skip to content

Improved long data type support#1311

Merged
robkeim merged 2 commits intoexercism:masterfrom
ErikSchierboom:long-parsing
Aug 19, 2019
Merged

Improved long data type support#1311
robkeim merged 2 commits intoexercism:masterfrom
ErikSchierboom:long-parsing

Conversation

@ErikSchierboom
Copy link
Copy Markdown
Member

This PR adds improved support for the long data type.

@ErikSchierboom ErikSchierboom requested a review from robkeim August 19, 2019 08:59
public void No_factors()
{
Assert.Empty(PrimeFactors.Factors(1));
Assert.Empty(PrimeFactors.Factors(1L));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've now made it clear that each input value is actually a long value. Previously, an implicit conversion was taking place, but this is more clear I feel.

Comment thread exercises/say/SayTest.cs
[Fact]
public void Zero()
{
Assert.Equal("zero", Say.InEnglish(0));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've now made it clear that each input value is actually a long value. Previously, an implicit conversion was taking place, but this is more clear I feel.

testMethod.Input["number"] = (long)testMethod.Input["number"];

if (!(testMethod.Expected is string))
if (testMethod.ExpectedIsError)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I couldn't resist a tiny cleanup :)


private static dynamic ConvertJObject(JObject jObject)
{
var properties = jObject.ToObject<IDictionary<string, dynamic>>();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This line was the main issue, as the conversion here would result in the dictionary not containing JToken values, which meant that the ConvertJToken functionality (which already had code to deal with long/int value conversions) was not called.

Interestingly, the new code is way easier!

Copy link
Copy Markdown
Contributor

@robkeim robkeim left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @ErikSchierboom!

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