Fixes #66: Adds support for SIDEBOARD_CONFIG_OVERRIDES env var by RobRuana · Pull Request #68 · magfest/sideboard

RobRuana · 2017-03-15T04:12:33Z

I am like 90% sure I am forgetting something. So please look over this and ask questions before approving it.

coveralls · 2017-03-15T04:16:46Z

Coverage increased (+0.3%) to 77.647% when pulling 6bc49e7 on fix-66-sideboard-config-overrides into aa40f7e on master.

kitsuta

This generally looks fine to me so I asked a bunch of dumb questions instead. :) I would like to see uniquify either return an ordered list or the docstring altered, though.

kitsuta · 2017-03-15T11:31:21Z

+
+    """
+    seen = set()
+    return [i for i in x if i not in seen and not seen.add(i)]


I'm not sure I'm understanding this list comprehension, particularly the not seen.add(i). How does a conditional work with a function like that? I went ahead and tested this in the IDE and it seems to work fine, I just don't know how.

On a separate note, what DOESN'T work is trying to uniquify a list that's out of order. It will not, as the docstring implies, order the list for you. E.g.:

>>> x = ['e', 'b', 'a', 'c', 'a', 'd', 'a', 'e'] >>> seen = set() >>> [i for i in x if i not in seen and not seen.add(i)] ['e', 'b', 'a', 'c', 'd']

On a separate note, what DOESN'T work is trying to uniquify a list that's out of order.

I think what Rob meant was that it would maintain the existing order rather than sort them. I've requested a clarification in the docstring.

I'm not sure I'm understanding this list comprehension, particularly the not seen.add(i).

The code is exactly equivalent to the following:

xs = [] for i in x: if i not in seen: xs.append(i) seen.add(i) return xs

The tricky part about implementing that as a list comprehension is the fact that we have to add the elements to seen as we go. However, since seen.add(i) adds the current element to the set and always returns None no matter what, we can add it as a conditional in our list comprehension.

Two suggestions about this function:

You should probably call listify on the parameter just to be safe. I'm a big fan if listify :)

Purely a style suggestion: I like the Haskell convention of using xs as the variable name for "a generic iterable" and x as a variable name for "a generic element". We use that convention elsewhere in Sideboard, and if you used it here then we'd say [x for x in listify(xs) if x not in seen and not seen.add(x)]. The main reason why I like that more than for i in x is that i is usually the variable name used for indexes, and I did a double take when first reading this function to make sure we were collecting elements rather than indexes.

Yeah, I figured a possibility might be that the function was working as intended but the docstring just implied something different. Updating the docstring is fine.

That's crazy that you can do that with list comprehension.

A few notes about this function:

It really belongs in sideboard.lib._util, but trying to import it from there causes a circular import error (even if I import it from inside a function).

I usually use "ordered" to mean the the order of elements is relevant and preserved, and "sorted" to mean in ascending or descending order. That being said this docstring is totally ambiguous and needs a rewrite.

It doesn't need to be a list comprehension at all, and would be clearer in long form as described above.

In my original implementation I DID call listify on the argument! But then I thought, if someone passes in an argument that is not already "listy" we should probably error on that. For example, if someone accidentally passes in None, then we would return [None], which I don't think is what we intend to do. So, for my perspective, if that's really what someone wants, then they can listify the argument before passing it to uniquify.

I used x as the parameter name because that's what we also use for listify and is_listy, but then I was like, "what variable am I gonna use for the items? Oh well, i it is!"

It really belongs in sideboard.lib._util, but trying to import it from there causes a circular import error (even if I import it from inside a function).

That makes sense because sideboard.lib assumes that sideboard.config has already been implemented. However, if we decide we want it to be importable in sideboard.lib then we could import it in sideboard/lib/__init__.py and then add it to that module's __all__. However, I'm not sure this is generally useful enough for us to put it there.

In my original implementation I DID call listify on the argument! But then I thought, if someone passes in an argument that is not already "listy" we should probably error on that. For example, if someone accidentally passes in None, then we would return [None], which I don't think is what we intend to do. So, for my perspective, if that's really what someone wants, then they can listify the argument before passing it to uniquify.

Reasonable! In that case I suggest importing sideboard.lib.is_listy adding a line like

assert is_listy(xs), 'uniquify requires a listy argument'

I used x as the parameter name because that's what we also use for listify and is_listy, but then I was like, "what variable am I gonna use for the items? Oh well, i it is!"

Also reasonable. The reason why we use x for listify and is_listy is because we're not sure whether it's already an iterable or not, so I thought x made more sense than xs there.

Good luck importing is_listy into config.py!

I'm actually a fan of list comprehension, so I don't think you need to explode it. It's pretty clear what this function does -- I just wanted to know exactly how it was working. It would be different if this was part of a more complex function, but because it's in its own function it is totally fine as-is.

Oh right! Fair enough, probably not worth the effort!

kitsuta · 2017-03-15T12:03:31Z

+    config_paths = []
+    for config_path in uniquify([s.strip() for s in config_overrides.split(';')]):
+        config_paths.append(config_path)
+        m = _defaults_re.match(config_path)


This (specifically _defaults_re) is some crazy regexp but I tested it in the IDE and it seems to work.

kitsuta · 2017-03-15T12:04:51Z

-    files as "root" and "module_root" and are available for interpolation.  For
-    example, a plugin could have a line in their config file like
+    files as "root" and "module_root" and are available for interpolation. For
+    example, a plugin could have a line in their config file like::


There's two semi-colons here.

I believe those are full colons. A lot of programming languages use full colons when only a semicolon would do :P

Oops, this is what I get for reviewing PRs before I'm fully awake. :P Yes, there's two colons here.

Wow! We've got some eagle-eyed reviewers here! I'm impressed. However, I did intentionally use two colons. In reStructuredText (which is the de facto standard for docstrings) two colons indicates the start of a literal block (as you'd use for code or other mono-space reformatted blocks).

Ahh, gotcha. Good to know, I'll start doing that myself in the future.

Neat! Confusing syntax on their part, but neat!

kitsuta · 2017-03-15T12:06:55Z

        from sideboard.lib import config
        plugin_name = os.path.basename(module_dir)
-        root_dir = os.path.join(config['plugins_dir'], plugin_name.replace('_', '-'))
+        root_dir = os.path.join(config['plugins_dir'], plugin_name)


What was the reason for getting rid of the replace here? That seems like something that could break some plugins if they were expecting underscores to be replaced with hyphens.

Indeed, we need to keep this replace. Suppose we have a Sideboard plugin called foo-bar. We wanted to follow the standard .deb and .rpm packaging conventions so that when building e.g. an RPM then we would have the package be named foo-bar.rpm rather than foo_bar.rpm, and thus it also looks in /etc/sideboard/plugins.d/foo-bar.cfg for its config file (which is what this line is doing).

Obviously Python modules need to use underscores, which means that a lot of our packaging would do things like drop our main module file into /opt/sideboard/plugins/foo-bar/foo_bar/__init__.py which seems a little silly but is how we've always done it.

MAGFest doesn't care about any of this, because we run everything out of cloned Git repos so we're not doing any actual packaging and don't look in /etc/sideboard for any config files whatsoever. But it's important for projects which do care about packaging, e.g. my day job :)

I wondered about this. We haven't settled on a standard here: underscores or hyphens in package names. It seems like you've settled on hyphens in your day job, which will probably end up driving this decision, but in our deployments we are still using underscores (for attendee_tournaments and uber_analytics). So we have something like this:

/opt/sideboard/plugins/attendee_tournaments/attendee_tournaments/__init__.py /opt/sideboard/plugins/uber_analytics/uber_analytics/__init__.py

I opted to remove the call to .replace() as it fits MAGFests current deployment scheme, and I've seen many python packages that use underscores in the package name (logging_unterpolation for example).

As a compromise, how about something like this?

root_dir = os.path.join(config['plugins_dir'], plugin_name) if '_' in plugin_name and not os.path.exists(root_dir): root_dir = os.path.join(config['plugins_dir'], plugin_name.replace('_', '-'))

That compromise sounds great!

kitsuta · 2017-03-15T12:09:50Z

            os.path.join(config_root, 'sideboard-core.cfg'),
-            os.path.join(config_root, 'sideboard-server.cfg'),
-        ]
+            os.path.join(config_root, 'sideboard-server.cfg')]


Nitpicky stylistic note: Since the opening square bracket is separated, I'd expect the ending square bracket to also be on its own line. It's harder to read if it's at the end of this line instead.

This is a classic style argument among programmers! FWIW, my preference is the same as @kitsuta but there doesn't seem to be a single agreed upon style convention for this, and in my not-statistically-significant sample of programmers I've worked with, I think Rob's "end on the same line" version is more common.

I am agnostic on this. Happy to change it!

kitsuta · 2017-03-15T12:12:50Z

        else:
            if not isinstance(value, uuid.UUID):
-                return '%.32x' % uuid.UUID(value)
+                return uuid.UUID(value).hex


It's not super-clear what this change is for (although it certainly looks more idiomatic at first glance).

I believe this was just code cleanup, since the latter seems like a much better approach to accomplish the same thing.

This actually fails on python 3.5 and above:

Python 3.6.0 (default, Jan 23 2017, 11:13:53) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import uuid >>> '%.32x' % uuid.uuid4() Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: %x format: an integer is required, not UUID

I ran into this error because I added python 3.5 to sideboard's test environments. It was a small enough change I didn't think it warranted a new issue and separate pull request.

kitsuta · 2017-03-15T12:14:40Z

+    assert ['a', 'b', 'c'] == uniquify(['a', 'b', 'c'])
+    assert ['a', 'b', 'c', 'd', 'e'] == uniquify(['a', 'b', 'a', 'c', 'a', 'd', 'a', 'e'])
+    assert ['a'] == uniquify(['a', 'a', 'a', 'a', 'a', 'a', 'a', 'a'])
+


I would also have a test here for a list that's out of order -- the test will currently fail (see above comment on uniquify).

kitsuta · 2017-03-15T12:18:19Z

+    assert ['a'] == uniquify(['a', 'a', 'a', 'a', 'a', 'a', 'a', 'a'])
+
+
+@pytest.mark.skipif(


I don't quite understand this test, since you later set or unset the overrides var during tests. Is the skip here just in case that monkeypatch fails, for some reason?

I wanted to write a test that ensured test-defaults.ini was being loaded if SIDEBOARD_CONFIG_OVERRIDES was set in the environment (independently of monkey patching it after the tests have already begun).

However, I didn't want the test to fail if someone just ran the tests from the command line without setting the SIDEBOARD_CONFIG_OVERRIDES, so that's why it's skipped.

This was mostly a sanity check for my own benefit. I'll add a docstring explaining why this test exists.

kitsuta · 2017-03-15T12:20:26Z

+        root_path = '/opt/sideboard'
+        return (module_path, root_path)
+
+    def test_get_module_and_root_dirs_plugin(self, plugin_dirs):


You're hard-coding paths in some places and using os.path in others. Will that cause these tests to fail if run in an unexpected environment?

Our use of os.path.join and os.path.sep is somewhat vestigial. Originally when we were coding Sideboard, we though, "We should use os.path.join on general principle in case we ever need to run this on Windows or something crazy like that." Fast forward to now and I think it's safe to say that Sideboard is definitely a Unix-based system and we simply don't ever intend to support it on any other platform. I still tend to use things like os.path.join anyway just out of habit and good general practice, but it's definitely not a problem to assume / as a path separator IMO.

I think at this point the forward slash '/' is supported as a separator on all platforms (even windows). But it doesn't matter because these hardcoded paths probably won't exist anyway. The functions being tested aren't actually examining the filesystem, they're just doing string manipulation.

The one exception to that is where you see os.getcwd() being used.

kitsuta · 2017-03-15T12:21:35Z

+
 [testenv]
+setenv="SIDEBOARD_CONFIG_OVERRIDES=test-defaults.ini"
+deps= -rrequirements.txt


Are the two 'r's here a typo?

I don't believe so. If memory serves, this basically says that we should pass the argument -r requirements.txt to pip.

EliAndrewC · 2017-03-15T14:35:07Z

+
+def uniquify(x):
+    """
+    Returns an ordered copy of `x` with duplicate items removed.


I would specify what "ordered" means in this context. From the implementation I can see that what you mean is "in the order in which the items originally appeared" but a casual reader might assume you meant "in sorted order".

EliAndrewC · 2017-03-15T14:55:40Z

+      expect in production
+
+
+    If `is_plugin` is `False` the first of the returned files is:


Confusing wording; this should probably read "the first two returned files are" since we're listing two files.

I should note, I changed the plugin parameter to is_plugin for clarity. Generally I'm fine with not using the is_* prefix to indicate a boolean, but for these methods in particular I was initially confused because I kept thinking that plugin was referring to a module, and I had keep going back to read the docstring.

EliAndrewC · 2017-03-15T15:09:01Z

Other than the one thing about the hyphen and a couple of style suggestions, this looks good to me!

kitsuta

+1

kitsuta · 2017-03-15T17:46:46Z

+
+    """
+    seen = set()
+    return [i for i in x if i not in seen and not seen.add(i)]


I'm actually a fan of list comprehension, so I don't think you need to explode it. It's pretty clear what this function does -- I just wanted to know exactly how it was working. It would be different if this was part of a more complex function, but because it's in its own function it is totally fine as-is.

kitsuta · 2017-03-15T17:47:31Z

-    files as "root" and "module_root" and are available for interpolation.  For
-    example, a plugin could have a line in their config file like
+    files as "root" and "module_root" and are available for interpolation. For
+    example, a plugin could have a line in their config file like::


Neat! Confusing syntax on their part, but neat!

kitsuta · 2017-03-15T17:49:32Z

-        ]
+            os.path.join(config_root, 'sideboard-server.cfg')]
+
+    override_configs = [os.path.join(root_dir, config_path) for config_path in get_config_overrides()]


Yeah, config_bases doesn't read very well, but I think it's fine to say base_configs + config_overrides. It's symmetrical, which is almost as good as being parallel!

coveralls · 2017-03-15T18:55:44Z

Coverage increased (+0.3%) to 77.688% when pulling 0df7cbb on fix-66-sideboard-config-overrides into aa40f7e on master.

RobRuana · 2017-03-15T18:53:13Z

+    - python: "3.4"
+      env: TOX_ENV=py34
+    - python: "3.5"
+      env: TOX_ENV=py35


This looks more complicated than necessary, but it's the one of the ways to guarantee Travis will find the correct interpreter. You'll find a lot of issues about this if you google "Travis InterpreterNotFound"; see this issue among many others.

RobRuana · 2017-03-15T18:54:32Z

-    tests/*
-
-include =
-    sideboard/*


Perhaps a controversial decision; I moved .coveragerc into setup.cfg just to cut down on the number of files in the project directory. I'm happy to put it back if anyone objects!

I believe that including this kind of thing in setup.cfg is considered more canonically correct these days, so this change seems good.

RobRuana · 2017-03-15T18:55:40Z


-def get_dirnames(pyname, plugin):
+def get_module_and_root_dirs(requesting_file_path, is_plugin):
    """


Function name changed just for clarity. Feel free to suggest a better name, or to object to renaming this function in general!

Fixes #66: Adds support for SIDEBOARD_CONFIG_OVERRIDES env var

6bc49e7

kitsuta reviewed Mar 15, 2017

View reviewed changes

EliAndrewC reviewed Mar 15, 2017

View reviewed changes

kitsuta approved these changes Mar 15, 2017

View reviewed changes

Addresses pull request review comments, squash before merging :)

0df7cbb

RobRuana commented Mar 15, 2017

View reviewed changes

RobRuana merged commit ed02745 into master Mar 15, 2017

RobRuana deleted the fix-66-sideboard-config-overrides branch March 15, 2017 19:38

EliAndrewC pushed a commit that referenced this pull request May 9, 2017

Fixes #66: Adds support for SIDEBOARD_CONFIG_OVERRIDES env var (#68)

50066a3

		assert ['a'] == uniquify(['a', 'a', 'a', 'a', 'a', 'a', 'a', 'a'])


		@pytest.mark.skipif(

		expect in production


		If `is_plugin` is `False` the first of the returned files is:

Conversation

RobRuana commented Mar 15, 2017

Uh oh!

coveralls commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kitsuta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coveralls commented Mar 15, 2017 •

edited

Loading

coveralls commented Mar 15, 2017 •

edited

Loading