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

Removed @objc from internal properties #3038

Merged
merged 1 commit into from Dec 5, 2017
Merged

Removed @objc from internal properties #3038

merged 1 commit into from Dec 5, 2017

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 23, 2017

Since the framework is entirely Swift, internal properties do not need to be marked as @objc

Since the framework is entirely Swift, internal properties do not need to be marked as @objc
@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #3038 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3038   +/-   ##
=======================================
  Coverage   19.77%   19.77%           
=======================================
  Files         113      113           
  Lines       13663    13663           
=======================================
  Hits         2702     2702           
  Misses      10961    10961
Impacted Files Coverage Δ
Source/Charts/Charts/PieRadarChartViewBase.swift 0% <ø> (ø) ⬆️
Source/Charts/Charts/RadarChartView.swift 0% <ø> (ø) ⬆️
Source/Charts/Highlight/ChartHighlighter.swift 1.78% <ø> (ø) ⬆️
...ource/Charts/Renderers/CombinedChartRenderer.swift 0% <ø> (ø) ⬆️
Source/Charts/Jobs/AnimatedViewPortJob.swift 0% <ø> (ø) ⬆️
Source/Charts/Jobs/ViewPortJob.swift 0% <ø> (ø) ⬆️
Source/Charts/Utils/Transformer.swift 47.5% <ø> (ø) ⬆️
Source/Charts/Renderers/RadarChartRenderer.swift 0% <ø> (ø) ⬆️
...arts/Data/Implementations/Standard/ChartData.swift 30.5% <ø> (ø) ⬆️
Source/Charts/Highlight/RadarHighlighter.swift 0% <ø> (ø) ⬆️
... and 13 more

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 bc40b16...6869d85. Read the comment docs.

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 24, 2017

@liuxuan30 This is a quick and easy one.

@liuxuan30
Copy link
Member

if it contains more than 3 files it's not a quick one lol

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 5, 2017

I remember we just added those tags since swift 4.

Internal access enables entities to be used within any source file from their defining module, but not in any source file outside of that module. You typically use internal access when defining an app’s or a framework’s internal structure.

Does this mean no one need access to these internals outside Charts framework anymore? If true, we can remove then.

For _xxx variables it's safe to remove @objc as they have their public getters. however for methods, it seems weird ObjC can actually call internal methods, so removing the tag or just mark it public seems better choice. In history we had a commit moving many interval methods and classes into open. and we can do it if necessary.

@liuxuan30 liuxuan30 self-requested a review December 5, 2017 01:55
@jjatie
Copy link
Collaborator Author

jjatie commented Dec 5, 2017

the internal access method means that the properties/methods can only be used within the Charts framework. Any consumer of the framework cannot see them.

Since any consumer of the framework cannot see them, and the framework is written in Swift internally, all internal and private functions and properties do not need to be marked as @objc because they will never be used by objective-c

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 5, 2017

yes, but the trick is adding @objc will let Objc see it and break the rules

@liuxuan30 liuxuan30 merged commit b87d04e into ChartsOrg:master Dec 5, 2017
@liuxuan30
Copy link
Member

liuxuan30 commented Dec 5, 2017

for math refining I have to delay one day. get back to work

@jjatie jjatie deleted the internal-drop-objc branch December 5, 2017 02:13
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.

None yet

3 participants