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

Remove public access modifier for internal class _URLEncodedFormEncoder #3053

Merged
merged 1 commit into from Jan 30, 2020
Merged

Remove public access modifier for internal class _URLEncodedFormEncoder #3053

merged 1 commit into from Jan 30, 2020

Conversation

mattt
Copy link
Sponsor Contributor

@mattt mattt commented Jan 29, 2020

Goals ⚽

Ensure consistency within the public API.

Implementation Details 🚧

When testing out swift-api-inventory on Alamofire, I saw this line in the output, which stuck out because of the leading underscore:

init _URLEncodedFormEncoder(context: URLEncodedFormContext, codingPath: [CodingKey], boolEncoding: URLEncodedFormEncoder.BoolEncoding, dataEncoding: URLEncodedFormEncoder.DataEncoding, dateEncoding: URLEncodedFormEncoder.DateEncoding)

Looking at the source, _URLEncodedFormEncoder is an internal type, but the initializer showed up in the inventory because of its public access modifier.

final class _URLEncodedFormEncoder {
var codingPath: [CodingKey]
// Returns an empty dictionary, as this encoder doesn't support userInfo.
var userInfo: [CodingUserInfoKey: Any] { return [:] }
let context: URLEncodedFormContext
private let boolEncoding: URLEncodedFormEncoder.BoolEncoding
private let dataEncoding: URLEncodedFormEncoder.DataEncoding
private let dateEncoding: URLEncodedFormEncoder.DateEncoding
public init(context: URLEncodedFormContext,

This change removes public to give that initializer an implicit internal access level to match its enclosing type _URLEncodedFormEncoder.

Testing Details πŸ”

I ran the test suite locally and await the CI to finish to confirm that this change doesn't break anything. For this kind of issue, it seems like a linter rule would be more appropriate than a unit test. Reading through SwiftLint's list of rules, I didn't see anything that does this yet. I'll ask around to see if folks think that would be helpful, and if so, write that up.

@cnoon cnoon self-requested a review January 29, 2020 20:37
@jshier
Copy link
Contributor

jshier commented Jan 29, 2020

This method isn't actually public, due to the type not being public, right? So really this seems like something the compiler should warn about, unless there's a use case I'm missing here.

In any case, thanks for this, we should be good to merge, as the test failures are those tests being flaky.

Copy link
Member

@cnoon cnoon left a comment

Choose a reason for hiding this comment

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

Thanks @mattt! 🍻

@mattt
Copy link
Sponsor Contributor Author

mattt commented Jan 29, 2020

This method isn't actually public, due to the type not being public, right? So really this seems like something the compiler should warn about, unless there's a use case I'm missing here.

I know, right? I was genuinely surprised that this didn't generate a compiler warning. You make a good point β€” I think I'll bring this up on the forums / file a bug first.

@jshier jshier merged commit e0a88a8 into Alamofire:master Jan 30, 2020
@jshier jshier added this to the 5.0.0 milestone Feb 14, 2020
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