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

Avoid exposing extra abstractions #458

Merged
merged 1 commit into from
Apr 3, 2013
Merged

Avoid exposing extra abstractions #458

merged 1 commit into from
Apr 3, 2013

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Apr 3, 2013

Explicitly getting a 2d view is not strictly necessary in these examples - I anticipate this is going to pose a bit of a documentation challenge.

tschaub added a commit that referenced this pull request Apr 3, 2013
Avoid exposing extra abstractions
@tschaub tschaub merged commit 7e7a071 into openlayers:master Apr 3, 2013
@tschaub tschaub deleted the fewer-abstractions branch April 3, 2013 18:45
@twpayne
Copy link
Contributor

twpayne commented Apr 3, 2013

Please note that I removed this commit with a push --force to master as it will break things and was merged without proper review.

@tschaub
Copy link
Member Author

tschaub commented Apr 4, 2013

Forcing a push to the default branch of a community project is a pretty rash move @twpayne. Our policy with example changes has been that they can be made without review - though I certainly don't want to be making changes that others on the project disagree with.

And I'm definitely curious to hear how map.getView().getView2D().setCenter() will continue to work in these examples while map.getView().setCenter() couldn't be made to work.

@twpayne
Copy link
Contributor

twpayne commented Apr 4, 2013

Forcing a push to the default branch of a community project is a pretty rash move @twpayne. Our policy with example changes has been that they can be made without review - though I certainly don't want to be making changes that others on the project disagree with.

I agree. Pushing breaking changes to the default branch without review is also a pretty rash move.

@bartvde
Copy link
Member

bartvde commented Apr 4, 2013

@twpayne as @tschaub said, in OL2 changes to the examples can be made without review. I assume the same policy applies to ol3?

Also, it's hard to know you're breaking something that is not even part of master as yet, unless I am misunderstanding something here?

@twpayne
Copy link
Contributor

twpayne commented Apr 4, 2013

@bartvde, this is already in master.

It is disingenuous to claim that because the change is in an example that is OK to make disputed changes. For example, is it OK to delete the vector examples without review because they don't work on most browsers?

@elemoine
Copy link
Member

elemoine commented Apr 4, 2013

One thing I wanted to say during today's hangout but could not: please no more push force to the main repo's master branch. If we think someone pushed something to master, and should not have, then let's create an issue (marked "severe" or something), discuss it, and possibly revert the commit. Let's not our anger control our Git actions :) Thanks.

@tschaub
Copy link
Member Author

tschaub commented Apr 4, 2013

And a final note for posterity. I was inspired to try the original change based on @elemoine's comment. After waiting to see that the build passed, I merged the commit. This all felt pretty standard (making a change to an example based on another core dev's comment, waiting for build to pass, merging). And I would say this is a very different move than a single dev unilaterally deleting all vector examples or something else of that nature.

I acknowledge that my commit comment may have raised tempers already on edge due to other difficult discussions. Perhaps we need Travis to offer suggestions like "take a walk" or "get some fresh air" before allowing merges.

In any case, until we all feel really good about our API, I'll treat the "trivial commit" policy less liberally. It is clear to me that we do all want the same thing (even though we keep pretending we're at odds).

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

4 participants