-
Notifications
You must be signed in to change notification settings - Fork 10
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
Refactor so that wrapping functionality can be reused more easily #7
Conversation
|
||
# Read input. | ||
lines = in_fp.readlines() | ||
class WrapInfo(NamedTuple): |
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.
can't say i like namedtuples in general, but i guess using dataclass
means this code won't work with python3.6 anymore...
macchiato.py
Outdated
if black_args is None: | ||
black_args = [] | ||
|
||
with tempfile.NamedTemporaryFile(suffix=".py", mode="wt+", delete=False) as fp: |
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.
while at it, maybe write the temp file file in the os.getcwd()
? see my comments in #4.
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.
Ok, I've made the change for this and added some tests to see if the config file is detected properly.
hey @alex-lee thanks for your interest. happy to merge this; left a few comments. this whole project started out as a really quick 'n dirty hack, and hence was just the simplest ‘stupid’ script that could work... but then i added support for new corner cases, and other people added more... 🙈 and while i'm not really comfortable treating this code as a ‘library’ (instead of a command line tool), in practice the changes needed for that are not that big anyway, and this pull request does that in a more than decent way (docstrings, type annotations, various cleanups, and so on). so yeah, |
If there is an existing project config, it will be detected and used.
Thanks for taking a look @wbolster and for the comments! Let me know if the working directory change and the related tests look ok. |
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.
this looks great, thanks!
fyi i pushed 1.3.0 with these changes to pypi: https://pypi.org/project/black-macchiato/1.3.0/ |
heh and looking at that page it reminded me of #3 🙈 |
I wanted to incorporate black-macchiato into a plugin, so I refactored the
macchiato
function to expose the wrapping functionality. I'm not sure if this is the sort of change you'd want to merge, but in case it looks worthwhile, here it is 🙂