Skip to content
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

QNTM-4876: Fix List.MaximumItem for mixed types #9012

Merged
merged 2 commits into from
Jul 24, 2018

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Jul 23, 2018

Purpose

Fixes Github issue: #9009
JIRA: QNTM-4876

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner

FYIs

@jnealb

@@ -1399,7 +1399,7 @@ private static IList IncreaseDepth(IList list, int amt)
/// </summary>
private static object DoubleConverter(object obj)
{
if (obj is int)
if (obj is int || obj is long)
Copy link
Member

Choose a reason for hiding this comment

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

is max item the only place where the double converter is used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

doubtful

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing a quick search in the code there are only two places in List.cs where DoubleConverter appears (other than the main class definition); MinimumItem and MaximumItem

Copy link
Contributor Author

@aparajit-pratap aparajit-pratap Jul 23, 2018

Choose a reason for hiding this comment

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

Yes. BTW this change is necessary after the switch from Int32 to Int64 types in DesignScript.

Copy link
Member

Choose a reason for hiding this comment

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

yep yep, just curious if we should be looking for other regressions like this throughout the code base.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mjkkirschner @aparajit-pratap Take a look at QNTM-3423 and all linked issues.

@mjkkirschner
Copy link
Member

LGTM - thanks @aparajit-pratap

@aparajit-pratap aparajit-pratap merged commit a9718dc into DynamoDS:master Jul 24, 2018
@aparajit-pratap aparajit-pratap deleted the list.maxItem branch July 24, 2018 00:50
alfarok pushed a commit to alfarok/Dynamo that referenced this pull request Nov 8, 2018
* fix List.MaximumItem for mixed types

* add DYN graph
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.

4 participants