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

Improve handling of file names and paths #144

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

enpaul
Copy link

@enpaul enpaul commented Sep 28, 2023

Thank you so much for making this project! Each commit in this PR is self contained and could be reviewed separately, let me know if there's anything I can do to make the review process easier.

Summary of changes:

  • Update DockerfileParser class to use pathlib.Path objects for file read/write operations
  • Add DockerfileParser.dockerfile attribute to expose the path to the Dockerfile as a pathlib.Path object while maintaining backwards compatibility with the existing DockerfileParser.dockerfile_path property as a string
  • Add dockerfile_filename keyword to DockerfileParser.__init__() to allow users to provide an alternative name for their dockerfile (ex: Dockerfile.dev, Dockerfile.sample, Containerfile, etc) while maintaining backwards compatibility with the existing default of Dockerfile
  • Add tests for these new features

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Looks good to me, just one backwards compatibility consideration and some unittest nitpicks

tests/test_parser.py Outdated Show resolved Hide resolved
tests/test_parser.py Show resolved Hide resolved
dockerfile_parse/parser.py Show resolved Hide resolved
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM except 1 linter error

the pylint and markdownlint errors are unrelated

dockerfile_parse/parser.py Outdated Show resolved Hide resolved
enpaul added 4 commits October 3, 2023 11:04
Add new attribute 'dockerfile' to the DockerfileParser class which exposes the path
to the dockerfile as a PathLib object
Add new property 'dockerfile_path' to the DockerfileParser class which exposes the
'dockerfile' attribute as a string for backwards compatibility

Signed-off-by: Ethan Paul <ethan.paul@portalinstruments.com>
Signed-off-by: Ethan Paul <ethan.paul@portalinstruments.com>
Signed-off-by: Ethan Paul <ethan.paul@portalinstruments.com>
Signed-off-by: Ethan Paul <ethan.paul@portalinstruments.com>
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