Add a Settings object to advance Twine having a Real API #365

sigmavirus24 · 2018-05-17T17:06:03Z

This begins the work described in #361 and is structured as 3 distinct commits:

c7b1f61 adds a helper that enables us to make certain elements of our API reject positional arguments. This is a shim designed to help us provide an API that forces users to communicate to future maintainers what they're doing.
cfdb15f adds the actual Settings object that will become part of our public API and can be used to replace the mess of parameters we're using in our commands
3ced656 actually begins using our Settings object internally in our commands to start the dog-fooding process.

I'm going to leave in-line comments as well to point out things that are interesting/potentially controversial.

This allows us to design the APIs so that they only take keyword arguments to make it easier and more explicit for users. Refs #361

This adds the Settings object described in our API specification so folks can get a feeling for how it will work and what it will be responsible for. Refs #361

Let's start internally using our new API and dogfood it. This will allow us to start understanding the impact of this new API. Refs #361

theacodes

Looks mostly fine. :)

theacodes · 2018-05-17T17:10:12Z

twine/settings.py

+        if repository_url.startswith((repository.LEGACY_PYPI,
+                                      repository.LEGACY_TEST_PYPI)):
+            raise exceptions.UploadToDeprecatedPyPIDetected(
+                "You're trying to upload to the legacy PyPI site '{0}'. "


Since this error message is always the same, why not move it into the UploadToDeprecatedPyPIDetected class?

That's a great idea! I can do that in this PR if you'd like or move it to another.

Since you're already touching this code, totally fine to do it here.

✔️ done

theacodes · 2018-05-17T17:13:16Z

tests/test_settings.py

+
+def test_settings_transforms_config(monkeypatch):
+    """Verify that the settings object transforms the passed in options."""
+    replaced_get_repository_from_config = pretend.call_recorder(


This amount of patching here makes me a little nervous. If we change any interfaces in settings.utils this test likely not fail, as the stubs here capture the interface at the time the tests were written. Can we accomplish this with less stubbing?

It bothered me as I was writing it. Part of this ties into the idea that Settings.__init__ does validation and other logic on top of initialization. This is why I was leaning more towards having an object that builds a Settings object. That would be simpler to avoid this altogether. At the same time, the proposal in #362 could also make this simpler to test without all of this obnoxious patching.

I was trying to avoid hitting stdin or the disk, in this case, but I'm also happy to use the method we have elsewhere of writing a temporary file with pypirc config in it and passing that to avoid having to do the stubbing. Let me know what you think.

If you're planning on revisiting this test later on, then LGTM. Otherwise, yeah, I would prefer to just turn it into an "integration" test. Also I know I'm a bit picky about tests so feel free to disregard.

(shameless self-promotion: I wrote a blog about some of this stuff - use real collaborators when possible, and if you can't, be real careful about how you mock/stub).

I'm picky too. I also tend to try to fit that into the existing strategy of the test suite if I'm taking over someone's project.

We're on the same page there. :)

✔️ Done

sigmavirus24 · 2018-05-17T17:07:04Z

tests/test_upload.py

-                      sign_with=None, config_file=pypirc, skip_existing=False,
-                      repository_url=None, verbose=None,
-                      )
+        upload_settings = settings.Settings(


This test shows that we've essentially made our validation of configuration settings/values more common than just the upload command. Future commits should rewrite this test and move it to tests/test_settings.py.

sigmavirus24 · 2018-05-17T17:08:07Z

tests/test_upload.py

@@ -178,12 +180,7 @@ def none_upload(*args, **kwargs):
               "TWINE_CERT": "/foo/bar.crt"}
    with helpers.set_env(**testenv):
        cli.dispatch(["upload", "path/to/file"])
-    cli.dispatch(["upload", "path/to/file"])


With much of the work separated out into the Settings object which is created and managed outside of the upload.upload function, our tests will error on this call since the Settings object otherwise attempts to retrieve the missing values.

sigmavirus24 · 2018-05-17T17:09:12Z

twine/commands/register.py


    # Call the register function with the args from the command line
-    register(**vars(args))
+    register(register_settings, args.package)


Our Settings class distinctly only cares about what it cares about. Extra arguments need to be handled separately by the user of the object. This is something I could see us revising to allow for a unified usage.

sigmavirus24 · 2018-05-17T17:10:35Z

twine/commands/upload.py


    args = parser.parse_args(args)
+    upload_settings = settings.Settings.from_argparse(args)


The change to the environment variable test in tests/test_upload.py stems from this. If instead we created the Settings object inside the upload function the one invocation would not error but the first would fail (since we'd never call Settings.from_argparse. In short, this indicates to me that the environment var test needs to be rewritten to accommodate both of these cases and should be moved to tests/test_settings.py

sigmavirus24 · 2018-05-17T17:11:49Z

twine/exceptions.py

@@ -13,33 +14,52 @@
 # limitations under the License.


-class RedirectDetected(Exception):
+class TwineException(Exception):


As we're working towards an API. I've updated all of Twine's exceptions to have a single root in TwineException. This makes API users' lives easier. They can use a catch-all

try: ... except twine.exceptions.TwineException: ...

Instead of trying to catch all of them. This can be removed from this PR if desired.

sigmavirus24 · 2018-05-17T17:13:22Z

twine/settings.py

+        self.config_file = config_file
+        self.comment = comment
+        self.skip_existing = skip_existing
+        self._handle_repository_options(


I think these methods will be the most controversial part of this.

On the one hand, I can imagine having a separate object, e.g., SettingsBuilder, that does this work and then makes Settings into a mostly data class that stores the validated/correct configuration and does the other nice things (like checking repository URLs and creating a new repository). I went with the simpler/thinner implementation to start with given the lack of feedback on the API design document, though.

sigmavirus24 · 2018-05-17T17:15:05Z

twine/settings.py

+
+    def create_repository(self):
+        """Create a new repository for uploading."""
+        repo = repository.Repository(


This logic was already being repeated, so I pulled it out to here. The same goes for the logic in check_repository_url.

codecov · 2018-05-17T17:22:39Z

Codecov Report

Merging #365 into master will increase coverage by 4.82%.
The diff coverage is 78.09%.

@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
+ Coverage   68.84%   73.66%   +4.82%     
==========================================
  Files          12       13       +1     
  Lines         597      638      +41     
  Branches       95       97       +2     
==========================================
+ Hits          411      470      +59     
+ Misses        158      142      -16     
+ Partials       28       26       -2

Impacted Files	Coverage Δ
twine/commands/register.py	`0% <0%> (ø)`	⬆️
twine/exceptions.py	`100% <100%> (ø)`	⬆️
twine/utils.py	`84.37% <100%> (+3.67%)`	⬆️
twine/commands/upload.py	`65.62% <40%> (-0.26%)`	⬇️
twine/settings.py	`91.22% <91.22%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ab1a60...b341906. Read the comment docs.

Since the message is always the same, let's keep it with the class to avoid any confusion.

sigmavirus24 · 2018-05-17T19:12:16Z

@theacodes did you want to take another look at this?

theacodes

LGTM!

sigmavirus24 added 3 commits May 16, 2018 08:30

Add no_positional decorator helper for planned API

c7b1f61

This allows us to design the APIs so that they only take keyword arguments to make it easier and more explicit for users. Refs #361

Add the Settings object per the API spec

cfdb15f

This adds the Settings object described in our API specification so folks can get a feeling for how it will work and what it will be responsible for. Refs #361

Simplify our existing commands with Settings

3ced656

Let's start internally using our new API and dogfood it. This will allow us to start understanding the impact of this new API. Refs #361

theacodes approved these changes May 17, 2018

View reviewed changes

sigmavirus24 commented May 17, 2018

View reviewed changes

Fix up the lint found by flake8

a2b7f21

sigmavirus24 added 2 commits May 17, 2018 12:29

Move default message into class definition

333a0a8

Since the message is always the same, let's keep it with the class to avoid any confusion.

Stop patching our utility functions

b341906

theacodes approved these changes May 17, 2018

View reviewed changes

sigmavirus24 merged commit f2695b0 into master May 17, 2018

sigmavirus24 deleted the settings-object branch May 17, 2018 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Settings object to advance Twine having a Real API #365

Add a Settings object to advance Twine having a Real API #365

sigmavirus24 commented May 17, 2018

theacodes left a comment

theacodes May 17, 2018

sigmavirus24 May 17, 2018

theacodes May 17, 2018

sigmavirus24 May 17, 2018 •

edited

Loading

theacodes May 17, 2018

sigmavirus24 May 17, 2018

theacodes May 17, 2018

sigmavirus24 May 17, 2018

theacodes May 17, 2018

sigmavirus24 May 17, 2018

sigmavirus24 May 17, 2018

sigmavirus24 May 17, 2018

sigmavirus24 May 17, 2018

sigmavirus24 May 17, 2018

sigmavirus24 May 17, 2018

sigmavirus24 May 17, 2018

sigmavirus24 May 17, 2018

codecov bot commented May 17, 2018 •

edited

Loading

sigmavirus24 commented May 17, 2018

theacodes left a comment


		args = parser.parse_args(args)
		upload_settings = settings.Settings.from_argparse(args)

Add a Settings object to advance Twine having a Real API #365

Add a Settings object to advance Twine having a Real API #365

Conversation

sigmavirus24 commented May 17, 2018

theacodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigmavirus24 May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 17, 2018 • edited Loading

Codecov Report

sigmavirus24 commented May 17, 2018

theacodes left a comment

Choose a reason for hiding this comment

sigmavirus24 May 17, 2018 •

edited

Loading

codecov bot commented May 17, 2018 •

edited

Loading