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

Show log(x).since combination in README #699

Merged
merged 11 commits into from Mar 19, 2024

Conversation

mattsalt
Copy link

@mattsalt mattsalt commented Mar 19, 2024

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

  • Ensure all commits include DCO sign-off.
  • Ensure that your contributions pass unit testing.
  • Ensure that your contributions contain documentation if applicable.

Description

  • Adds some more detail to the README for the log function, specifically around combining with other filters.

.log.since respects the default count configuration which makes sense but isn't clear in the examples or the docs.
Perhaps naively I expected g.log.since('1 month ago') to return a whole month's worth of commits. Instead it consistently returned 30 which is the default for the count param. To return a whole month you have to also specify a greater maximum i.e. g.log(1000).since('1 month ago') depending on the amount of activity in the repository.

jcouball and others added 10 commits January 13, 2024 17:03
* Update min required version of Ruby and Git

Signed-off-by: James Couball <jcouball@yahoo.com>
Signed-off-by: James Couball <jcouball@yahoo.com>
Signed-off-by: James Couball <jcouball@yahoo.com>
* Refactor the Error heriarchy
* Bump truffleruby to 24.0.0 to get support for endless methods

Signed-off-by: James Couball <jcouball@yahoo.com>
* Implement the new timeout feature
Signed-off-by: James Couball <jcouball@yahoo.com>
Signed-off-by: Georgiy Melnikov <melnikov@evrone.com>
Signed-off-by: James Couball <jcouball@yahoo.com>
@mattsalt mattsalt force-pushed the log-readme-details branch 2 times, most recently from 9811e6b to b7e8c75 Compare March 19, 2024 08:59
Signed-off-by: Matthew Salt <matthew.salt@gmail.com>
@mattsalt mattsalt marked this pull request as ready for review March 19, 2024 09:32
@jcouball
Copy link
Member

The build failure for TruffleRuby and JRuby on Windows are expected to fail for the git-2.x gem.

You are merging to master which is the git-2.x pre-release branch. There is a 1.x branch for the 1.x release line.

@mattsalt mattsalt changed the base branch from master to v1 March 19, 2024 16:21
Copy link
Member

@jcouball jcouball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @mattsalt.

Approved.

I am thinking of changing the default to return all commits with the 2.x release.

@example Return all commits that meet the critieria
  git = Git.open('.')
  commits = git.log

@example Return at most 30 commits
  git = Git.open('.')
  commits = git.log

The thinking is that is the way that git itself works. I think that the (undocumented) principle of this gem is to provide an interface that adhears closely to the git CLI.

Comments welcome.

@jcouball jcouball merged commit 3724833 into ruby-git:v1 Mar 19, 2024
8 of 10 checks passed
@jcouball
Copy link
Member

Since you changed your base branch to 'v1' the documentation won't show on the default Github README.md since it is on the default branch of master. Sorry I just noticed that as I hit the merge button.

@jcouball
Copy link
Member

@mattsalt I merged this PR without realizing that when you changed the base, it pulled all the commits from master that were not also in v1.

Since I caught this issue within a few minutes of merging the change, I ended up force-pushing the v1 branch back to what it was before your PR. You will need to update your local repository to match. Hopefully I caught this before it impacts anyone else.

Sorry for the mess up on my part for not catching this in my review. I'll add your change in a new PR in just a bit.

That said, since the change is to the README, I think you do want it on the master branch since that is what people see in the Github UI by default. But to atone for my sins, I'll add it in both branches.

@jcouball
Copy link
Member

Ok, your change has been merged on both master and v1.

Again, sorry for my mistake and the resulting confusion.

@mattsalt
Copy link
Author

Thank you! Apologies, I changed the base branch after what you mentioned in the first comment. I saw the unit tests were still failing so was going to check on that but you're clearly far too quick for me.

@mattsalt
Copy link
Author

Thank you for the contribution @mattsalt.

Approved.

I am thinking of changing the default to return all commits with the 2.x release.

@example Return all commits that meet the critieria
  git = Git.open('.')
  commits = git.log

@example Return at most 30 commits
  git = Git.open('.')
  commits = git.log

The thinking is that is the way that git itself works. I think that the (undocumented) principle of this gem is to provide an interface that adhears closely to the git CLI.

Comments welcome.

Personally that would have been easier to understand as a new user of the library. However once you do understand there is a limit it easy to use with the default.

That said matching the git CLI experience would definitely reduce the onboarding load. I imagine most people using this library for any log related activity would be looking at more than 30 items.

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

3 participants