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

Request for Feedback: API stability and beta functionality #178

Open
mickeyreiss opened this issue Apr 8, 2017 · 7 comments
Open

Request for Feedback: API stability and beta functionality #178

mickeyreiss opened this issue Apr 8, 2017 · 7 comments
Assignees

Comments

@mickeyreiss
Copy link
Contributor

Context

Recent PRs (e.g. #177, #176, #144) have brought forth API changes that seem like a great idea, but are not necessarily validated to be useful.

If they are pulled in and turn out to be a mistake, it's impossible to revert due to backwards compatibility. This slowly erodes the quality and simplicity of the library.

However, if these PRs are rejected, we risk missing out on useful functionality, and it hurts future open source engagement from that contributor.

Current Process

Currently, I make a subjective decision whether or not to expand the API to include these methods. I'd prefer to make these decisions more objectively and base them on actual community preferences and adoption.

Suggestions

  1. Add a mechanism for testing out new signatures in the API. For example, a beta annotation in comments, or a method name prefix like autoBeta.... Then, after some period of time, make a decision to pull a feature into the supported API, or remove the beta functionality.

  2. Add a mechanism to measure community usage of various functionality. For example, write a shell script that counts the number of invocations of each method in your codebase and uploads it to a survey.

@revolter
Copy link
Member

Well, personally, I felt they were supposed to already exist. I tried to use autocomplete to find them and was surprised they weren't there. But I understand the fear of adding them for just a portion of the users.

About the suggestions:

  1. Are you saying to actually release new versions with beta prefixed methods? Pulling them in would be like making breaking backwards compatibility changes, as the people using them would have to update the codebase.
  2. For this, you need to first release with the new additions, then wait for everybody to notice them or something.

@aceontech
Copy link

ReactiveCocoa used to have a separate "community" repository, https://github.com/RACCommunity/Rex, where they would put extensions and experimental features.

You could do the same for PureLayout, in turn, only exposing the new/experimental APIs via a separate extension library/pod until they get officially merged into the main repository.

@revolter
Copy link
Member

revolter commented May 9, 2017

But the question is, which APIs are extensions and which should have been from the beginning? I could argue that, at least some of the ones I added, are not extensions. And, obviously, that could be just my opinion, but they could also be useful to other users that didn't saw them yet.

@revolter
Copy link
Member

Any news with this one? I still stumble upon the missing APIs from this library almost daily. And it's not only a personal preference, it's also about the consistency. As I already mentioned, the axis alignment already have a "same axis" method, so the edge alignment should have it too. Also, I don't think it increases the library's size incredibly much that it becomes a problem.

Needing to align, for example, the top edge of two views is a common thing when you can't use a stack view.

@toohotz
Copy link
Member

toohotz commented May 9, 2018

Another possible (though rather simplistic seeming) solution would be to give this beta API its own branch to live off of. Though there come side effects of course of now having to manage essentially two different codebases (over time).

The topic of useful vs being a mistake can be seen in two different ways to myself. If the code is correct in what it is achieving then the worst case is that these are additional APIs that are not being used frequently by the community. On the other hand if what you are saying is that the code is incorrect then yes, once it is out in the world it would erode the quality of PL to have to retract an API that is not functioning as how it was designed. Commit 04a3 is an example of this where the code quality code quality took a momentary dip as a follow up PR had to fix the incorrect change that was made.

Ultimately, I'm not opposed to the the first option of having these beta functionality added into the code base though when in consideration, we should decide on a per API basis the usefulness of these APIs. We should also for these PRs that will be made, have more than one member of the team give their approval so that more eyes can be on the code to ensure its quality. As a plus, it would be nice as well for any new APIs that are being added to the codebase that there are tests that verify before drafting the PR that it at least works as to how the contributor intends it to.

@revolter
Copy link
Member

The thing that feels inconsistent to me is that some type of constraints do have shorthand APIs (e.g. autoAlignAxis:toSameAxisOfView:) while others don't (e.g. autoPinEdge:toSameEdgeOfView:). Could it be a bad idea to have the same set of APIs for each constraint type? So if you can align an axis to the superview or a specific view, shouldn't we also be able to pin an edge to the superview or a specific view?

I'm not a fan of a beta release/branch, as most probably very few people would use them, and we wouldn't know at all how many use them. We either add them (properly reviewed and tested) either don't (though it looks to me as the API is subjective/opinionated instead of generic/abstract).

Some examples from my codebase:


[view1 autoAlignAxis:ALAxisHorizontal toSameAxisOfView:view2];
[view1 autoAlignAxis:ALAxisVertical toSameAxisOfView:view2];

could be replaced with:

[view1 autoCenterWithView:view2];

(view1 and view2 are siblings in the hierarchy)


[view1 autoPinEdgeToSuperviewEdge:ALEdgeTop];
[view1 autoPinEdgeToSuperviewEdge:ALEdgeRight];
[view1 autoPinEdgeToSuperviewEdge:ALEdgeBottom];

could be replaced with:

[view1 autoPinAllEdgesToSuperviewEdgesExcept:ALEdgeLeft];

[view1 autoMatchDimension:ALDimensionWidth toDimension:ALDimensionWidth ofView:view2];
[view1 autoMatchDimension:ALDimensionHeight toDimension:ALDimensionHeight ofView:view2];

could be replaced with:

[view1 autoMatchDimensionsToDimensionsOfView:view2];

[view1 autoPinEdge:ALEdgeTop toEdge:ALEdgeTop ofView:view2];
[view1 autoPinEdge:ALEdgeLeading toEdge:ALEdgeLeading ofView:view2];

could be replaced with:

[view1 autoPinEdge:ALEdgeTop toSameEdgeOfView:view2];
[view1 autoPinEdge:ALEdgeLeading toSameEdgeOfView:view2];

@lwdupont
Copy link
Contributor

I'm all for more methods to help developers. The ones mentioned seem fine to me too. Any opposition to me doing some merging? :)

@lwdupont lwdupont self-assigned this Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants