Skip to content

Conversation

@wael-mahlous
Copy link

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

tools/git.py is not currently covered by tests.

What does this PR do?

It exposes the repo object (https://gitpython.readthedocs.io/en/stable/reference.html#git.repo.base.Repo), which in turn can be used for various assertions, as demonstrated by the new test, or perhaps even for logging. Previously, only the head was exposed (https://gitpython.readthedocs.io/en/stable/reference.html#git.repo.base.Repo.head).

References

Issue #8

How has this PR been tested?

The tests were run locally. Coverage was also run locally and increased as a result of the new test.

Is this a breaking change?

No

Does this PR require an update to the documentation?

No

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

As per issue neuroinformatics-unit#8, the git-related code is not being tested. This commit can be a starting point for git-related tests. It exposes the repo object, which in turn can be used for various assertions.

Completed during UCL ARC's Winter Open-Source Good First Issue Hackathon.

Co-authored-by: Matthew Scroggs <[email protected]>
Copy link
Member

@adamltyson adamltyson left a comment

Choose a reason for hiding this comment

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

Hi @wael-mahlous, thanks for this, much appreciated!

Looks good, I've just left a few small comments.

"tox",
"pre-commit",
"setuptools_scm",
]
Copy link
Member

Choose a reason for hiding this comment

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

This is needed to ensure that git is installed when setting up a development environment.

Suggested change
"fancylog[git]"
]

from rich.logging import RichHandler

import fancylog
import fancylog.tools.git
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really needed as fancylog is imported above.

Suggested change
import fancylog.tools.git

A gitpython repo object.
"""
self.repo = repo
self.head = GitHead(repo.head.commit)
Copy link
Member

Choose a reason for hiding this comment

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

Just to use this variable consistently

Suggested change
self.head = GitHead(repo.head.commit)
self.head = GitHead(self.repo.head.commit)

assert f"{'pytest':20} {'1.1.1'}"


def test_git_info():
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a short description to explain what this text is doing?

@wael-mahlous
Copy link
Author

@adamltyson thanks a lot for your comments and apologies for the late response (due to conflicting priorities). Will address all of these by end of the week!

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