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

Trim API #507

Merged
merged 4 commits into from
Feb 12, 2016
Merged

Trim API #507

merged 4 commits into from
Feb 12, 2016

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Feb 7, 2016

this is done in an effort to stabilize the API for SwiftLint 1.0 (#479).

Still needs:

  • discussion: It occurred to me as I was marking more APIs as internal that anything used by rule implementations in this framework would benefit external consumers of SwiftLintFramework if they're also writing their own rules. However, since we haven't focused on that experience so far, maybe extensibility should explicitly be a non-goal for 1.0, since it inherently collides directly with stability. Hence the need for discussion around this.
  • changelog entry: Pending discussion to see what we actually want to do here.

@jpsim
Copy link
Collaborator Author

jpsim commented Feb 7, 2016

I'd also like us to consider making the following changes:

  • remove the Configuration.disabledRules property: this is only really used for unit testing, and can be computed rather than stored.
  • make Configuration.configPath public: I think it's a useful property of the configuration for external consumption. It's even used in the == implementation (correctly so IMO), which generally means that it should be public. Otherwise, a consumer of Configuration might have two identical values as far as they can see, but that aren't equal.
  • consider disabbreviating "config": e.g. RuleConfig, setConfig, etc? IMO we should spell out configuration. Semi-related, it seems that applyConfiguration is more correct than setConfiguration. (doing this in Expand usage of "config" to "configuration" in most places #525)

@jpsim jpsim force-pushed the jp-trim-api branch 2 times, most recently from a3f53e0 to 4ee94b2 Compare February 9, 2016 19:55
@jpsim jpsim changed the title adjust access control levels for many APIs in SwiftLintFramework Trim API Feb 9, 2016
@jpsim
Copy link
Collaborator Author

jpsim commented Feb 11, 2016

@scottrhoyt @norio-nomura do you have any thoughts on this? Since there are so many "breaking" changes to the API in here, I'd like to get this in before cutting a new release.

@norio-nomura
Copy link
Collaborator

consider disabbreviating "config"

👍

@scottrhoyt
Copy link
Contributor

@jpsim I agree on the non-goal of external SwiftLintFramework consumers. I'd rather over-trim here and set that as a 2.0 goal so that we have some more wiggle room in 1.x--especially with more maturity in the package manager/linux support and 2.2/3.0 around the corner.

I'm all for disabbreviating configs as it's probably more in line with the Swift API guidelines (though I don't necessarily think config is out of line considering it's common parlance for most developers). I would have done that initially except I was running into quite a few line_length violations, so I abbreviated and tried to be somewhat consistent without changing all the existing Configurations. applyConfiguration does make more sense as well.

@jpsim
Copy link
Collaborator Author

jpsim commented Feb 11, 2016

I think this PR is complete. Care to review @scottrhoyt & @norio-nomura?

Notably, I'm curious if I missed any APIs that we could make internal, private or remove altogether.

@norio-nomura
Copy link
Collaborator

Looks good to me. 👍

jpsim added a commit that referenced this pull request Feb 12, 2016
@jpsim jpsim merged commit f468a24 into master Feb 12, 2016
@jpsim jpsim deleted the jp-trim-api branch February 12, 2016 18:53
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.

3 participants