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

Git log getter #36

Merged
merged 1 commit into from
Feb 9, 2019
Merged

Git log getter #36

merged 1 commit into from
Feb 9, 2019

Conversation

Asenar
Copy link
Contributor

@Asenar Asenar commented Jan 21, 2019

This PR contains getCommitMessage and getCommitData method to interrogate git repository history.

@janpecha
Copy link
Contributor

Thanks for PR. For now I have 2 questions:

  1. what is use-case for standalone method getCommitMessage?
  2. wouldn't be better merge getCommitMessage with getCommitData into single method getCommit() ?

I'm thinking about something like this:

  • new method getLog($limit, $offset = 0): array - it returns array of commit ID

  • method getCommit($commitId): Commit - it returns instance of object Commit($commitId, $author, $commiter, $message) or throws exception for wrong commit ID

But I have never need it so it's hard to me imagine real use-cases and design good API.

What do you think?

@Asenar
Copy link
Contributor Author

Asenar commented Jan 23, 2019

Use-case for standalone getCommitMessage : when I just want to get the last commit message without anything else without additional process. But I can change it to protected if you think it's better.

git show return a lot of data and the output is potentially altered by a local or system-wide .gitconfig. So it's really difficult to get only the commit message, that's why I put it in a separate method.

That's also the reason why I put a foreach to get the data I needed instead of «guessing» the first line would be the author, the 3rd would be the date, …

About getLog($limit, $offset) I think it should be named getCommitsHash() or something else, because «log» refers more to the message than the hash. + there is a lot more options than limit and offset (I personally use always --merges --branches and --all, without mentioning --graph). Also limiting commit by path seems pretty useful.

About a Commit object, it could be interesting, but seems a lot of works to cover all information (getParent, getChildren, cherry-pick, reword, …)

@Asenar
Copy link
Contributor Author

Asenar commented Jan 24, 2019

Hi, I just noticed I didn't pushed my final commit, so I updated the getCommitData() methods, and that can give a use case for getCommitMessage : I might want to show one commit (sha1 + first line), or a list of them, then an ajax request to get and display more data onclick (that's what I do, except I currently don't need the ajax part as I show only one commit).

Do you want I rename getCommitData to getCommit ? I'm not sure if it's relevant, as it doesn't return a commit object but an associative array containing some commit information (that can evolve in future), but it can surely be a future evolution.

Or I can name it that way if you prefer :)

@janpecha
Copy link
Contributor

janpecha commented Feb 6, 2019

Thanks! I added some commits:

  • I changed indentation from spaces to tabs
  • I modified PHPDoc
  • I changed commiter to committer
  • I removed argument $format - I think it's useless for now and can be added in future if it will be needed

Are these changes ok for you? If yes, I will squash commits and merge it into master.

@Asenar
Copy link
Contributor Author

Asenar commented Feb 7, 2019

Hi,

thanks you for your fixes, I didn't keep attention to tabs / spaces and other norms from that project, I will try to do it for further contributions :)

I notice you also replaced [] array style by array().

@janpecha janpecha merged commit 25b9bce into czproject:master Feb 9, 2019
@janpecha
Copy link
Contributor

janpecha commented Feb 9, 2019

Merged, thanks!

@janpecha janpecha mentioned this pull request Feb 9, 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

Successfully merging this pull request may close these issues.

2 participants