new setupcfg writer #204
new setupcfg writer #204
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
- Coverage 96.37% 96.26% -0.12%
==========================================
Files 87 88 +1
Lines 3996 4065 +69
==========================================
+ Hits 3851 3913 +62
- Misses 145 152 +7
|
|
I made this PR ready to review despite the codecov failure which is valid but due to my attempt to catch exceptions. After review I"ll go back and add some testing or skip coverage in certain lines. |
There was a problem hiding this comment.
Just curious but did you investigate whether this project would help simplify the implementation? https://github.com/Preocts/commented-configparser
I tested with commented-configparser and am very slightly disappointed that it's only a tiny bit buggy. It does retain the comments in most cases, but in some cases it moves comments to different lines. It also added some trailing spaces to the new setup.cfg file. So here's a diff: #comment
[metadata]
name = my_package
version = attr: my_package.VERSION
author = Josiah Carberry
author_email = [email protected]
description = My package description
long_description = file: README.rst, CHANGELOG.rst, LICENSE.rst
keywords = one, two
# comment should stay in ln 10
license = BSD-3-Clause
-classifiers =
+classifiers =
+# comment should stay in ln 16
Framework :: Django
Programming Language :: Python :: 3
-# comment should stay in ln 16changes are:
@drdavella lmk if you want to accept these changes and I can switch to using commented config or we can keep code as is |
|
@clavedeluna thanks for the info. I'm sorry it didn't work out of the box. Let's accept this as-is for now and we can revisit later since I don't think this is the most critical work for us right now. One other possibility down the road would be to vendor in that third-party code (assuming a permissive license) and maybe tweak it to get the behavior we need. |
| configparser does not retain formatting or comment lines, so we have to build | ||
| the output newline manually. | ||
| """ | ||
| clean_lines = list(map(lambda s: s.strip(), original_lines)) |
There was a problem hiding this comment.
I like map but genuinely curious whether there's any benefit to doing it this way rather than using a comprehension?
clean_lines = [s.strip for s in original_lines]There was a problem hiding this comment.
no other benefit to map, I think the moment I wrote it that's the first thing that came to me. I'll switch it out
| assert setup_cfg.read() == dedent(updated_setupcfg) | ||
|
|
||
|
|
||
| def test_dont_add_existing_dependency(tmpdir): |
There was a problem hiding this comment.
We should have another test case where the version of security that already exists in the package is different from the one being added. There should be no new dependency added (or version changed) in this case either.
There was a problem hiding this comment.
This is a really good test case to think about. None of the other writers handle for this either so if it's ok with you it would be good to handle it in a separate PR for all. It would be worth thinking about it a bit more: do we really not want to update the version? What if the project has pkg==1.0 but our recommendation is pkg==2.0?
Worth thinking about it independently.
|
Overview
Adds a setup.cfg file writer for new dependencies
Description
configparseronly to detect the existing dependencies in the setup.cfg file. We can't rely on it to update and write to file becauseconfigparserdoesn't retain some formatting and comment lines.