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

Resubmit of #2730 #3002

Merged
merged 5 commits into from
Dec 24, 2017
Merged

Resubmit of #2730 #3002

merged 5 commits into from
Dec 24, 2017

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 14, 2017

Resubmit of #2730

@codecov-io
Copy link

codecov-io commented Nov 14, 2017

Codecov Report

Merging #3002 into master will increase coverage by 0.64%.
The diff coverage is 70.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3002      +/-   ##
==========================================
+ Coverage      20%   20.64%   +0.64%     
==========================================
  Files         113      114       +1     
  Lines       15529    15431      -98     
  Branches      249      256       +7     
==========================================
+ Hits         3106     3186      +80     
+ Misses      12387    12208     -179     
- Partials       36       37       +1
Impacted Files Coverage Δ
...s/Data/Implementations/Standard/ChartDataSet.swift 37.08% <0%> (ø) ⬆️
Source/Charts/Charts/BarLineChartViewBase.swift 22.56% <0%> (+0.03%) ⬆️
.../Implementations/Standard/ChartDataEntryBase.swift 31.7% <0%> (+23.81%) ⬆️
Tests/Charts/EquatableTests.swift 100% <100%> (ø)
...Data/Implementations/Standard/ChartDataEntry.swift 36.73% <63.63%> (+30.32%) ⬆️
Source/Charts/Highlight/Highlight.swift 41.33% <81.81%> (+41.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b18e76...0ef8aa1. Read the comment docs.

@pmairoldi
Copy link
Collaborator

With the revert to isEqual seems like methods are just moved around. Is there any meaningful change anymore?

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 15, 2017

  • Use of == in the framework as per Swift guidelines
  • removed the == implementations as noted by the Objective-C/Swift interoperability guide
  • simplified the isEqual implementations for ChartDataEntryBase and Highlight
  • removed dependence of the super class in the ChartDataEntry isEqual implementation

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 23, 2017

@petester42 Any further feedback on this one or can it be merged?

@jjatie jjatie mentioned this pull request Nov 23, 2017
@jjatie jjatie merged commit 428843f into ChartsOrg:master Dec 24, 2017
@jjatie jjatie deleted the equatable-fixed branch December 24, 2017 18:23
@liuxuan30
Copy link
Member

@jjatie we still need a review process for what coming into master.

@jjatie
Copy link
Collaborator Author

jjatie commented Dec 25, 2017

Agreed. This was a PR that has had feedback back and forth since August. On the PR this one replaced, we agreed this would be merged in after the Swift 4 migration. I made the adjustments requested from the review, and this has just been sitting here since.

This PR removes a few mistaken == implementations, and takes advantage of the Swift Equatable protocol without changing any implementations. Seemed safe to merge after all this time.

@@ -0,0 +1,45 @@
//
// EquatableTests.swift
Copy link
Member

@liuxuan30 liuxuan30 Dec 25, 2017

Choose a reason for hiding this comment

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

I made the same template as yours for combined chart test. Shall we include the license info just like source code template, or test file is fine with default Xcode template?
The old test file does not have a template as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we should be putting in the license info to be consistent. Do we keep it the same, but whose name do we use?

Copy link
Member

@liuxuan30 liuxuan30 Dec 25, 2017

Choose a reason for hiding this comment

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

I would just use the same template as source code. I don't know the rule of names.
Personally I just use Copyright 2015 Daniel Cohen Gindi & Philipp Jahoda, and I don't need to specify who created the file.
I think this is a good question, as I saw some files with yours template:

//
//  DemoBaseViewController.swift
//  ChartsDemo-iOS
//
//  Created by Jacob Christie on 2017-07-03.
//  Copyright © 2017 jc. All rights reserved.
//

I guess we need an unified one. @petester42 @danielgindi @PhilJay insights?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I just use the Xcode default.

@danielgindi
Copy link
Collaborator

Just one thing: Please conform to the overall style of the library.
I created this library with braces in new lines as I believe it makes for the cleanest and most readable code, and when there's mixed use of braces up and down - it creates a braces hell.

In any codebase that I participate - I use the style of that codebase, braces up or down, spaces or no spaces, var naming etc.

@liuxuan30
Copy link
Member

@danielgindi how about the file template about license and names?

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.

5 participants