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

Consider refactoring to protocol extensions #387

Closed
DanielAsher opened this issue Feb 19, 2017 · 15 comments
Closed

Consider refactoring to protocol extensions #387

DanielAsher opened this issue Feb 19, 2017 · 15 comments

Comments

@DanielAsher
Copy link

DanielAsher commented Feb 19, 2017

Hi,

as a follow on from #386 I wanted to know if any consideration has been given to refactoring the framework to use protocol extensions. This could allow per-file limited imports of specific features.

As an example, currently Array.get is defined as:

extension Array {

    ///EZSE: Get a sub array from range of index
    public func get(at range: ClosedRange<Int>) -> Array {
        var subArray = Array()
        let lowerBound = range.lowerBound > 0 ? range.lowerBound : 0
        let upperBound = range.upperBound > self.count - 1 ? self.count - 1 : range.upperBound
        for index in lowerBound...upperBound {
            subArray.append(self[index])
        }
        return subArray
    }
    /// EZSE: Gets the object at the specified index, if it exists.
    public func get(at index: Int) -> Element? {
        guard index >= 0 && index < count else { return nil }
        return self[index]
    }

This seems to be imported into a file without any explicit import EZSwiftExtensions declaration.

Could this be refactored to:

public protocol EZArray : Collection { 
}

extension EZArray where Self.Index == Int, Self.IndexDistance == Int  {

    public func get(at index: Int) -> Self.Iterator.Element? {
        guard index >= 0 && index < count else { return nil }
        return self[index]
    }
    
    public func get(at range: ClosedRange<Int>) -> Array<Self.Iterator.Element> {
        var subArray = Array<Self.Iterator.Element>()
        let lowerBound = range.lowerBound > 0 ? range.lowerBound : 0
        let upperBound = range.upperBound > self.count - 1 ? self.count - 1 : range.upperBound
        for index in lowerBound...upperBound {
            subArray.append(self[index])
        }
        return subArray
    }
}

which would allow a file to either

  1. import protocol EZSwiftExtensions.EZArray (limited import)
  2. import EZSwiftExtensions (full import)

I'm not sure this would solve the problem (I don't really understand the subtleties of swift's import mechanism) or why a file would import EZSwiftExtensions without an explicit import EZSwiftExtensions declaration.

Thanks again!

@Khalian
Copy link
Collaborator

Khalian commented Feb 19, 2017

@DanielAsher I get your concern. I believe the namespace pollution comes from adding the library as a target dependency in your code. Alternatively if you use something like pods, IIRC they pull the source down and there is a compilation conflict.

My only concern is breaking the existing userspace. @goktugyil @lfarah @piv199 - What do you guys think. If we take this up, we would have to dedicate an entire release for this.

@Esqarrouth
Copy link
Owner

This would require heavy refactoring, will change the behaviour for all users. And I'm not sure if will actually work for all cases.

We try to differentiate extensions from standard libraries and make it clear with the docs that its an extension to minimize the problem.

@lfarah
Copy link
Collaborator

lfarah commented Feb 19, 2017

Can we at least find a quick fix for #386's forEach?

@Khalian
Copy link
Collaborator

Khalian commented Feb 19, 2017

I presume the fix for this would be to remove this

/// EZSE: Iterates on each element of the array.
@available(*, deprecated: 1.6, renamed: "forEach(_:)")
public func each(_ call: (Element) -> Void) {
    forEach(call)
}

If yes I will remove it ASAP.

@lfarah
Copy link
Collaborator

lfarah commented Feb 19, 2017

But if it's deprecated, why is @DanielAsher getting an error?

@Khalian
Copy link
Collaborator

Khalian commented Feb 19, 2017

I dont know why he is still getting an error.

#389

@DanielAsher
Copy link
Author

@Khalian - that for the quick PR and merge. I believe that deprecated extension methods still override standard library implementations.

Could you remove the following deprecated extension method from the library:

    @available(*, deprecated: 1.7, renamed: "forEachEnumerated(_:)")
    public func forEach(_ call: @escaping (Int, Element) -> Void) {
        forEachEnumerated(call)
    }

this should complete and close the issue.

Thanks!

@Khalian
Copy link
Collaborator

Khalian commented Feb 20, 2017

Ok will do

@Khalian
Copy link
Collaborator

Khalian commented Feb 20, 2017

#392

@DanielAsher
Copy link
Author

One more! Please remove deprecated removeAll
So sorry didn't report this earlier.

@Khalian
Copy link
Collaborator

Khalian commented Feb 21, 2017

Removed removeAll in PR.

@DanielAsher
Copy link
Author

It looks like Swift 3.1 will greatly simplify refactoring to finely-grained protocol extensions with https://bugs.swift.org/browse/SR-1009 being implement. Constrained extensions allow same-type constraints so refactoring StringExtension.swift simply requires:

public protocol EZString {}

extension String : EZString {}

extension EZString where Self == String {
    /// EZSE: Character count
    public var length: Int {
        return self.characters.count
    }
/// ...
}

@lfarah
Copy link
Collaborator

lfarah commented Mar 9, 2017

Hey @DanielAsher, that looks really cool!

@lfarah
Copy link
Collaborator

lfarah commented Mar 9, 2017

Can we close this, @Khalian?

@Khalian
Copy link
Collaborator

Khalian commented Mar 12, 2017

@lfarah We could but @DanielAsher has requested we deprecate : https://github.com/goktugyil/EZSwiftExtensions/blob/928d825dcc5002f468d8cfc0b94b98a5910f160c/Sources/ArrayExtensions.swift#L135

We can have a quick discussion on the merits and demerits of the same.

@Khalian Khalian closed this as completed Aug 7, 2017
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

No branches or pull requests

4 participants