-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add support for YAML config files #46
Conversation
* Increase the clarity of the usage documentation * Add the `requirements.txt` dependency file * Include the PyYAML package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution here 💯
I have not tested this locally, but the changes look good 💯, I have left some comments regarding minor changes to the documentation. :)
It would be great if you could include a sample YAML
config in the documentation :) https://github.com/saadmk11/changelog-ci#example-config-file
try: | ||
with open(config_file, 'r') as config_json: | ||
config = json.load(config_json) | ||
file = open(filepath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to close the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to close the file?
TL;DR No.
Explanation:
Once the reference count of file
reaches zero, which happens at the end of this try
block, CPython will call the __del__()
method of the underlying path-like object, which makes the call to close()
therein.
An argument can be made that how Python manages memory depends on which Python you're using. Most people use CPython, which uses this reference count system, so we have the guarantee that the file will receive the necessary call to close()
.
Since this is a GitHub Action which is run within a Docker container, we know we're using CPython, so I don't think it's worth changing this line of code to preserve niche cross-Python compatibility concerns, like maintaining support for Jython. 🎤💧
Good question though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the garbage collector will eventually close the file, but Isn't it good practice to always explicitly close the file without relying on the garbage collector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it is good practice to do so, but my response was answering your first question:
Don't we need to close the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, Alright 👍 The PR is almost ready to get merged 💯, I think we should close the opened file here 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it! You have write access to this branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm merging this to another branch before merging to master as I need to test the YAML config as well, thanks for your great work on this :)
Wow you have eagle eyes! Good catch! 🐛✋🏻🦅 Co-authored-by: Maksudul Haque <saad.mk112@gmail.com>
* Resolve PEP 8 "Package and Module Names" * Resolve PEP 8 (E131) "Hanging indentation" * Add preliminary support for Docker Compose
I don't think we need docker-compose for this action as it only runs one container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉🎉
Good point. Compose is truly, as you said, better suited for multi-container applications. |
requirements.txt
dependency file