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

Tree navigation: return objects #924

Closed
jdavid opened this issue Jul 7, 2019 · 7 comments
Closed

Tree navigation: return objects #924

jdavid opened this issue Jul 7, 2019 · 7 comments

Comments

@jdavid
Copy link
Member

jdavid commented Jul 7, 2019

@rcoup

Follow up from #903 (comment)

To avoid the reference from TreeEntry to the repository, the / operator could return an object instead of a TreeEntry.

Then this example from PR #903

feature_info = commit.tree / 'meta' / 'features' / 'info.json'
info = json.loads(feature_info.obj.data)

Would become:

feature_info = commit.tree / 'meta' / 'features' / 'info.json'
info = json.loads(feature_info.data)

It's slightly shorter and in my opinion more intuitive. Just tried with GitPython and apparently it behaves this way.

@jdavid
Copy link
Member Author

jdavid commented Aug 15, 2019

With commit @0d79ad8 now the / operator returns a Tree instead of a TreeEntry. Also added new Tree.name and Tree.filemode.

Next will be to remove the / operator from TreeEntry. Eventually the TreeEntry type should be removed, though that will break backwards compatibility.

@rcoup
Copy link
Contributor

rcoup commented Aug 30, 2019

Sorry, missed this in the post-baby-leave email swamps.

This is great!

I guess question is how to deal with compatibility? The #903 stuff isn't in a release, so there's a few options:

  1. break compatibility, bump to a new major version
  2. make mytree / path always return objects, and leave mytree[path], mytree[index] and iter(mytree) to return/use TreeEntry objects (and raise a DeprecationWarning). I think [] isn't so bad (just annoying) but we'd need an alternative method for iter() (items/iteritems/entries/iterentries/iterdir?).
  3. different class (hand-wavey) of Tree you get from the top (commit.get_tree() for arguments sake), which works exclusively using trees & objects, and leave the existing commit.tree using TreeEntry as it was in 0.28 & deprecate it.
  4. something else?

Personally I'd prefer (3) or (1), but I've only just started using pygit2 so I don't need to do a massive rewrite of old code ;)

@jdavid
Copy link
Member Author

jdavid commented Sep 4, 2019

I would declare TreeEntry obsolete, since the same information can be obtained from objects now. Or do you see a reason to keep TreeEntry?

If we can make objects largely compatible with TreeEntry, we could just stop returning tree entries and return objects instead. Then remove the TreeEntry type. It should be possible that the only change is that we return a different type. A warning in the release notes would be enough, as we've done in the past, and just release 0.28.3

PD: When the user is only interested in the tree-entry information, loading the object would be sub-optimal. Sometime in the future we may add lazy loading of the object to address this.

@jdavid
Copy link
Member Author

jdavid commented Sep 22, 2019

I'm working on this, in progress..

jdavid added a commit that referenced this issue Sep 22, 2019
@rcoup
Copy link
Contributor

rcoup commented Sep 23, 2019

Sounds reasonable. Maybe make TreeEntry a parent or mixin class so is-a relationships still work; or implementation-wise, where name/filemode come from?

If I get a Tree by ID, I presume name/filemode aren't populated? Because the same tree can appear multiple places, right?

foo1/        1ab23cd4
  bar/       ab1234cd
    wiz      abcd1234
foo2/        12abcd34
  bar/       ab1234cd
    wiz      abcd1234

In this example my understanding is that ab1234cd is the same Tree, but appearing in two places with two TreeEntrys?

jdavid added a commit that referenced this issue Oct 5, 2019
@jdavid
Copy link
Member Author

jdavid commented Oct 5, 2019

Right now I think the next release will be 1.0.0, there will be some small breaking changes, so I won't bother to keep the TreeEntry type.

Yes, the filemode/name come from git_tree_entry, so if you get the tree by id they will be missing.

There're several ways to do comparison, we'll use comparison by id, so they will be the same tree.

@jdavid
Copy link
Member Author

jdavid commented Oct 6, 2019

merged

@jdavid jdavid closed this as completed Oct 6, 2019
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

No branches or pull requests

2 participants