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

added methods for committer, sha, branch, commit message #136

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benmarwick
Copy link

An attempt to address #135. This is the first time I've worked with S4 objects so please let me know how to make it better. Probably some redundant @export lines in there...

@benmarwick
Copy link
Author

Thanks very much for your detailed comments. I'll work on them and make some more commits to this PR.

@krlmlr
Copy link
Contributor

krlmlr commented Jun 6, 2018

@stewid: Accessors become important to ensure compatibility between the S4 version 0.21.0 and the new S3 code. Is it too late to release 0.21.1 that has these accessors (like in the S4-classes branch) which then could be used to write code that works both with git2r 0.21.1 and subsequent S3-based releases?

It might be possible to auto-generate these accessors from the S4 class definitions, and then change them to S3 (in the master branch) with some search and replace. But we need tests too. Prefix for the new functions: git_get_ or ggit_ or gitg_?

For tic and other code, I'm working around this problem. Asking here because it might be of interest to other git2r users and dependent packages.

@stewid
Copy link
Member

stewid commented Jun 6, 2018

Accessors become important to ensure compatibility between the S4 version 0.21.0 and the new S3 code. Is it too late to release 0.21.1 that has these accessors (like in the S4-classes branch) which then could be used to write code that works both with git2r 0.21.1 and subsequent S3-based releases?

That was initially my plan (r-lib/devtools#1719). However, I don't think it's realistic anymore to release 0.21.1, since version 0.21.0 has a CRAN check warning on macOS that is related to building the bundled libgit2. In the master branch, the configuration has been refactored to use a system installation of libgit2 (if available). Unfortunately, it's not as simple as copying the configuration from the master branch to the S4-classes branch since 0.21 is using internal functionality in libgit2 not available in a system installation. A recent improvement in the master branch is that it's only using functions in the public libgit2 API #345 #344 #336. So I guess it's to much work to copy those changes over to the S4-classes branch.

@krlmlr
Copy link
Contributor

krlmlr commented Jun 7, 2018

Thanks for the heads-up. I'm fine with my workaround: https://github.com/ropenscilabs/tic/blob/a2e69a39401d30952319d59a1a34237d1e85da16/R/git2r.R.

@stewid
Copy link
Member

stewid commented Jun 7, 2018

Nice, it's an elegant workaround.

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