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 warnings on current code base #4321

Merged
merged 2 commits into from
Apr 16, 2020
Merged

Fix warnings on current code base #4321

merged 2 commits into from
Apr 16, 2020

Conversation

liuxuan30
Copy link
Member

Fix warnings for vars;
Fix unused instants;
Remove a duplicated function NSUIMainScreen()

Fix unused instants
Remove a duplicated function NSUIMainScreen()
@@ -39,11 +39,6 @@ extension UIScreen
}
}

func NSUIMainScreen() -> NSUIScreen?
Copy link
Member Author

@liuxuan30 liuxuan30 Mar 30, 2020

Choose a reason for hiding this comment

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

@jjatie I found this func is duplicated during half pie chart review. They are repeated under two macros iOS & macOS; I manually tested on my iOS simulator and macOS, it seems we only need one outside these macros. We are safe to delete one right? I don't come up a case that we need to put them into two places

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, we don't need both. Really this should be an extension on NSUIScreen

extension NSUIScreen {
    var nsuiMain: Self? { .main }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@jjatie I have pushed a new commit. but mine is

class var nsuiMain: NSUIScreen? { .main }

is there any reason to use Self? here? Because if I use Self?, Xcode would complain

Type 'Self?' has no member 'main'

and I have to use something like

class var nsuiMain: Self? { NSUIScreen.main as? Self }

(I can't even use { .main as? Self } ) So I think use NSUIScreen explicitly makes it cleaner.

@liuxuan30 liuxuan30 requested a review from jjatie March 30, 2020 07:13
@codecov-io
Copy link

codecov-io commented Mar 30, 2020

Codecov Report

Merging #4321 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4321      +/-   ##
==========================================
- Coverage   40.92%   40.91%   -0.02%     
==========================================
  Files         124      124              
  Lines        9456     9454       -2     
==========================================
- Hits         3870     3868       -2     
  Misses       5586     5586              
Impacted Files Coverage Δ
Source/Charts/Utils/Platform.swift 0.00% <ø> (ø)
Source/Charts/Renderers/PieChartRenderer.swift 65.20% <100.00%> (-0.13%) ⬇️
...ts/Renderers/XAxisRendererHorizontalBarChart.swift 55.67% <100.00%> (ø)
Source/Charts/Renderers/YAxisRenderer.swift 53.92% <100.00%> (ø)
...ts/Renderers/YAxisRendererHorizontalBarChart.swift 48.70% <100.00%> (ø)

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 56effcf...0b0d7ac. Read the comment docs.

@gligorkot
Copy link

@liuxuan30 would be good to have this merged in and released so we can eliminate XCode 11.4 warnings. 👍

@liuxuan30 liuxuan30 merged commit 915becb into master Apr 16, 2020
@liuxuan30 liuxuan30 deleted the FixWarnings branch April 16, 2020 07:17
@gligorkot
Copy link

Champion! Thank you @liuxuan30

mosaic-engineering pushed a commit to mosaic-io/Charts that referenced this pull request Apr 17, 2020
* Fix warnings for vars;
Fix unused instants
Remove a duplicated function NSUIMainScreen()

* use extension to replace NSUIMainScreen()
mosaic-engineering pushed a commit to mosaic-io/Charts that referenced this pull request Jun 17, 2020
* Fix warnings for vars;
Fix unused instants
Remove a duplicated function NSUIMainScreen()

* use extension to replace NSUIMainScreen()
SwiftPolar pushed a commit to SwiftPolar/Charts that referenced this pull request Mar 20, 2023
* Fix warnings for vars;
Fix unused instants
Remove a duplicated function NSUIMainScreen()

* use extension to replace NSUIMainScreen()
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