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

fix the wrong type error of == fuction #9

Open
wants to merge 2 commits into
base: release
Choose a base branch
from

Conversation

dreamnice
Copy link

No description provided.

@dreamnice
Copy link
Author

20200518004423

Copy link
Member

@winksaville winksaville 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 a couple suggestions here, plus I think tests should be added.

@@ -177,7 +177,7 @@ class FixedPixelSpaceOrdinalScaleSpec implements OrdinalScaleSpec {
}

@override
bool operator ==(Object other) => other is SimpleOrdinalScaleSpec;
bool operator ==(Object other) => other is FixedPixelSpaceOrdinalScaleSpec;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should compare not only the type but also any fields defined so something like:

  @override
  bool operator ==(Object other) {
    if (other is FixedPixelSpaceOrdinalScaleSpec) {
      return pixelSpaceBetweenBars ==
        (other as FixedPixelSpaceOrdinalScaleSpec).pixelSpaceBetweenBars;
    } else {
      return false;
    }
  }

@@ -198,7 +198,7 @@ class FixedPixelOrdinalScaleSpec implements OrdinalScaleSpec {
}

@override
bool operator ==(Object other) => other is SimpleOrdinalScaleSpec;
bool operator ==(Object other) => other is FixedPixelOrdinalScaleSpec;
Copy link
Member

Choose a reason for hiding this comment

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

Dito here:

  @override
  bool operator ==(Object other) {
    if (other is FixedPixelOrdinalScaleSpec) {
      return pixels == (other as FixedPixelOrdinalScaleSpec).pixels;
    } else {
      return false;
    }
  }

@winksaville
Copy link
Member

@dreamnice, I don't think the == Operator implemention is correct, see my review code comments.

I've got a proposal for a test file I suggest it should be placed in a in directory "test/chart/cartesina/axis/spec". Below is a start I think it would be good if we added tests for the other classed in the file:

// Copyright 2020 the Charts project authors. Please see the AUTHORS file
// for details.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import 'package:charts_common_cf/src/chart/cartesian/axis/spec/ordinal_axis_spec.dart';

import 'package:test/test.dart';


void main() {

  FixedPixelSpaceOrdinalScaleSpec fixedPixelSpaceOrdinalScaleSpec1;
  FixedPixelSpaceOrdinalScaleSpec fixedPixelSpaceOrdinalScaleSpec2;
  FixedPixelSpaceOrdinalScaleSpec fixedPixelSpaceOrdinalScaleSpec3;

  FixedPixelOrdinalScaleSpec fixedPixelOrdinalScaleSpec1;
  FixedPixelOrdinalScaleSpec fixedPixelOrdinalScaleSpec2;
  FixedPixelOrdinalScaleSpec fixedPixelOrdinalScaleSpec3;


  setUp(() {
    fixedPixelSpaceOrdinalScaleSpec1 = FixedPixelSpaceOrdinalScaleSpec(20);
    fixedPixelSpaceOrdinalScaleSpec2 = FixedPixelSpaceOrdinalScaleSpec(20);
    fixedPixelSpaceOrdinalScaleSpec3 = FixedPixelSpaceOrdinalScaleSpec(25);
    fixedPixelOrdinalScaleSpec1 = FixedPixelOrdinalScaleSpec(20);
    fixedPixelOrdinalScaleSpec2 = FixedPixelOrdinalScaleSpec(20);
    fixedPixelOrdinalScaleSpec3 = FixedPixelOrdinalScaleSpec(25);
  });

  group('== operator', () {
    test('SimpleOrdinalScaleSpec', () {
      SimpleOrdinalScaleSpec simpleOrdinalScaleSpec1 = SimpleOrdinalScaleSpec();
      SimpleOrdinalScaleSpec simpleOrdinalScaleSpec2 = SimpleOrdinalScaleSpec();

      expect(simpleOrdinalScaleSpec1 == simpleOrdinalScaleSpec2, equals(true)); 

      // Verify order doesn't matter
      expect(simpleOrdinalScaleSpec1 != fixedPixelSpaceOrdinalScaleSpec1, equals(true));
      expect(fixedPixelSpaceOrdinalScaleSpec1 != simpleOrdinalScaleSpec1, equals(true));
    });

    test('FixedPixelSpaceOrdinalScaleSpec', () {
      expect(fixedPixelSpaceOrdinalScaleSpec1 == fixedPixelSpaceOrdinalScaleSpec2, equals(true)); 
      expect(fixedPixelSpaceOrdinalScaleSpec1 != fixedPixelSpaceOrdinalScaleSpec3, equals(true)); 

      // Verify order doesn't matter
      expect(fixedPixelSpaceOrdinalScaleSpec1 != fixedPixelOrdinalScaleSpec1, equals(true));
      expect(fixedPixelOrdinalScaleSpec1 != fixedPixelSpaceOrdinalScaleSpec1, equals(true));
    });

    test('FixedPixelOrdinalScaleSpec', () {
      expect(fixedPixelOrdinalScaleSpec1 == fixedPixelOrdinalScaleSpec2, equals(true)); 
      expect(fixedPixelOrdinalScaleSpec1 != fixedPixelOrdinalScaleSpec3, equals(true)); 

      // Verify order doesn't matter
      expect(fixedPixelOrdinalScaleSpec1 != fixedPixelSpaceOrdinalScaleSpec1, equals(true));
      expect(fixedPixelSpaceOrdinalScaleSpec1 != fixedPixelOrdinalScaleSpec1, equals(true));
    });
  });

}

Copy link
Member

@winksaville winksaville left a comment

Choose a reason for hiding this comment

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

Need to update hashCode methods

@@ -177,7 +177,7 @@ class FixedPixelSpaceOrdinalScaleSpec implements OrdinalScaleSpec {
}

@override
bool operator ==(Object other) => other is SimpleOrdinalScaleSpec;
bool operator ==(Object other) => other is FixedPixelSpaceOrdinalScaleSpec;

@override
int get hashCode => 37;
Copy link
Member

Choose a reason for hiding this comment

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

This should return a hashCode based on pixelSpaceBetweenBars since that field is now used in operator ==.

@@ -198,7 +198,7 @@ class FixedPixelOrdinalScaleSpec implements OrdinalScaleSpec {
}

@override
bool operator ==(Object other) => other is SimpleOrdinalScaleSpec;
bool operator ==(Object other) => other is FixedPixelOrdinalScaleSpec;

@override
int get hashCode => 37;
Copy link
Member

Choose a reason for hiding this comment

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

This should return a hashCode based on the pixels field.

@winksaville
Copy link
Member

FYI: Here's a couple ways I run just one test file:

wink@3900x:~/prgs/flutter/projects/benchmarkx_gui/submodules/charts_cf/charts_common_cf (Fix-some-NaNs-when-window-is-small)
$ pub run test/chart/cartesian/axis/spec/ordinal_scale_test.dart 
00:00 +0: == operator SimpleOrdinalScaleSpec

00:00 +1: == operator FixedPixelSpaceOrdinalScaleSpec

00:00 +2: == operator FixedPixelOrdinalScaleSpec

00:00 +3: All tests passed!

wink@3900x:~/prgs/flutter/projects/benchmarkx_gui/submodules/charts_cf/charts_common_cf (Fix-some-NaNs-when-window-is-small)
$ pub run test test/chart/cartesian/axis/spec/ordinal_scale_test.dart 
00:01 +3: All tests passed!    

To run all of the tests in a single project:

wink@3900x:~/prgs/flutter/projects/benchmarkx_gui/submodules/charts_cf/charts_common_cf (Fix-some-NaNs-when-window-is-small)
$ pub run test
00:42 +420: All tests passed! 

And to run all of the tests in charts_cf:

wink@3900x:~/prgs/flutter/projects/benchmarkx_gui/submodules/charts_cf (Fix-some-NaNs-when-window-is-small)
$ make test
00:25 +420: All tests passed!                                                                                                                          
00:02 +28: All tests passed!          

@winksaville
Copy link
Member

Looks good, txs!

When you have time, will you be able to add support for hashCode and tests?

@dreamnice
Copy link
Author

Looks good, txs!

When you have time, will you be able to add support for hashCode and tests?

Can you tell me what the hashCode means, I have never used it before.

@winksaville
Copy link
Member

Can you tell me what the hashCode means, I have never used it before.

Every dart pbject inherits hashCode from Object and is primarily used by HashMap.

https://api.dart.dev/stable/2.8.4/dart-core/Object/hashCode.html
https://dart-lang.github.io/linter/lints/hash_and_equals.html

A google search for .

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