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

Provide treeView service API #1162

Merged
merged 11 commits into from Oct 18, 2017

Conversation

Projects
None yet
5 participants
@UziTech
Contributor

UziTech commented Aug 13, 2017

Description of the Change

Provided a service that lets other packages consume the treeView instance.

At the moment any package that interacts with the tree-view package to get selected files or other information are doing so by getting the active package's mainModule or checking classes and data attributes on elements.

There is a standard way to get this information through the Services API

Alternate Designs

We may want to change what actually get provided if there are unstable features in the tree-view module.

Benefits

Other packages can interact with this package in a standard way.

Possible Drawbacks

none

Applicable Issues

#433

@UziTech UziTech referenced this pull request Aug 20, 2017

Open

Provide a service API #433

@50Wliu 50Wliu added the needs-review label Aug 22, 2017

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Sep 13, 2017

Contributor

Here is an example of why this would be useful:

https://github.com/subesokun/atom-tree-view-git-status/blob/master/lib/main.coffee#L154

The tree-view-git-status package uses

atom.packages.getActivePackage('tree-view').mainModule

to get the treeView instance. This poses two problems:

  1. It will break if the module changes the way it stores the treeView instance. (As it seems to have done in v1.18.0)
  2. It limits tree-view-git-status to only work with the tree-view package instead of also working with any fork of the tree-view package.
Contributor

UziTech commented Sep 13, 2017

Here is an example of why this would be useful:

https://github.com/subesokun/atom-tree-view-git-status/blob/master/lib/main.coffee#L154

The tree-view-git-status package uses

atom.packages.getActivePackage('tree-view').mainModule

to get the treeView instance. This poses two problems:

  1. It will break if the module changes the way it stores the treeView instance. (As it seems to have done in v1.18.0)
  2. It limits tree-view-git-status to only work with the tree-view package instead of also working with any fork of the tree-view package.
@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Sep 13, 2017

Member

Thanks for the contribution! I do however have concerns. Your proposition exposes the entire tree-view instance, which does not feel like an appropriate solution. The only justification I can think of is that it's the easiest approach with the least amount of effort. Keep in mind that most (if not all) of the tree-view object consists of private methods.

I propose that we expose an actual API, instead of simply handing developers an entire object, which essentially has no lifetime guarantees etc. For instance, if someone relies on this object, and the tree-view package is disabled, what do you think/expect will happen?

To begin with, which aspects of the tree-view do you think we have to expose? Let's begin with the bare minimum of features that are actually needed by external packages rights now — YAGNI™ and so on.

Member

thomasjo commented Sep 13, 2017

Thanks for the contribution! I do however have concerns. Your proposition exposes the entire tree-view instance, which does not feel like an appropriate solution. The only justification I can think of is that it's the easiest approach with the least amount of effort. Keep in mind that most (if not all) of the tree-view object consists of private methods.

I propose that we expose an actual API, instead of simply handing developers an entire object, which essentially has no lifetime guarantees etc. For instance, if someone relies on this object, and the tree-view package is disabled, what do you think/expect will happen?

To begin with, which aspects of the tree-view do you think we have to expose? Let's begin with the bare minimum of features that are actually needed by external packages rights now — YAGNI™ and so on.

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Sep 13, 2017

Contributor

I agree, exposing the entire tree-view instance is probably not the right way to start out.

There are already some methods in tree-view class that are marked public. I'm assuming those are supposed to be public to the rest of the package. Those seem unlikely to change in any major way.

The most common thing other packages seem to do is get the selected paths. If we wanted to start really small we should start with that.

Contributor

UziTech commented Sep 13, 2017

I agree, exposing the entire tree-view instance is probably not the right way to start out.

There are already some methods in tree-view class that are marked public. I'm assuming those are supposed to be public to the rest of the package. Those seem unlikely to change in any major way.

The most common thing other packages seem to do is get the selected paths. If we wanted to start really small we should start with that.

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Sep 13, 2017

Member

The most common thing other packages seem to do is get the selected paths. If we wanted to start really small we should start with that.

OK, that sounds like a reasonable approach 🥇
Expose that functionality via a wrapper object. Ideally something that can withstand the tree-view being disabled, but doing something simple like we do in status-bar is probably acceptable as well. We can always improve that part later without breaking any dependent package.

Member

thomasjo commented Sep 13, 2017

The most common thing other packages seem to do is get the selected paths. If we wanted to start really small we should start with that.

OK, that sounds like a reasonable approach 🥇
Expose that functionality via a wrapper object. Ideally something that can withstand the tree-view being disabled, but doing something simple like we do in status-bar is probably acceptable as well. We can always improve that part later without breaking any dependent package.

UziTech added some commits Sep 14, 2017

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Sep 14, 2017

Contributor

I added getEntry as well for packages like tree-view-git-status that want to edit the elements in tree view.

getEntry sounds kind of weird to me. Should I change it to getElement or something like that?

Contributor

UziTech commented Sep 14, 2017

I added getEntry as well for packages like tree-view-git-status that want to edit the elements in tree view.

getEntry sounds kind of weird to me. Should I change it to getElement or something like that?

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Oct 6, 2017

Contributor

@thomasjo anything else holding this up?

Contributor

UziTech commented Oct 6, 2017

@thomasjo anything else holding this up?

@thomasjo

@UziTech Sorry about the delay on this. I added some additional comments, and I pinged @atom/feedback for some extra 👀 on this.

I think we're pretty close to being able to hit The Big Green Button™ on this.

Show outdated Hide outdated spec/tree-view-package-spec.coffee
Show outdated Hide outdated spec/tree-view-package-spec.coffee
Show outdated Hide outdated package.json
@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Oct 17, 2017

Member

This LGTM! Thanks for not giving up, @UziTech 😜


@atom/maintainers I'll give everyone a few hours to chime in on this, then I'll merge and ship this.

Member

thomasjo commented Oct 17, 2017

This LGTM! Thanks for not giving up, @UziTech 😜


@atom/maintainers I'll give everyone a few hours to chime in on this, then I'll merge and ship this.

@thomasjo thomasjo self-assigned this Oct 17, 2017

@50Wliu

50Wliu approved these changes Oct 17, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 17, 2017

Contributor

@thomasjo You cool documenting this in the README before merging?

Contributor

nathansobo commented Oct 17, 2017

@thomasjo You cool documenting this in the README before merging?

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Oct 18, 2017

Member

@nathansobo Sure thing! Good call 🥇

Member

thomasjo commented Oct 18, 2017

@nathansobo Sure thing! Good call 🥇

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Oct 18, 2017

Member

@UziTech You mind providing Atom maintainers push access to your fork? That wait I can push my commits directly to your fork before merging.

Member

thomasjo commented Oct 18, 2017

@UziTech You mind providing Atom maintainers push access to your fork? That wait I can push my commits directly to your fork before merging.

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Oct 18, 2017

Contributor

@thomasjo "Allow edits from maintainers." is checked is there anything else I need to do?

Contributor

UziTech commented Oct 18, 2017

@thomasjo "Allow edits from maintainers." is checked is there anything else I need to do?

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Oct 18, 2017

Member

Travis is being slow, so I'm going to merge this since the build was green prior to my documentation (and argument name tweak).

@UziTech Thanks for this! 🙇

Member

thomasjo commented Oct 18, 2017

Travis is being slow, so I'm going to merge this since the build was green prior to my documentation (and argument name tweak).

@UziTech Thanks for this! 🙇

@thomasjo thomasjo merged commit 1867638 into atom:master Oct 18, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 19, 2017

Contributor

Well now, looks like we're another step closer to tree-view services providing DOM access. ;) Perhaps this should be the next API addition to review?

Contributor

Alhadis commented Oct 19, 2017

Well now, looks like we're another step closer to tree-view services providing DOM access. ;) Perhaps this should be the next API addition to review?

@UziTech UziTech deleted the UziTech:provide-service branch Jan 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment