Add a configuration command #4240

pradyunsg · 2017-01-18T03:02:50Z

This is a configuration command for managing configuration files for pip through pip's CLI. It makes configuration more discoverable and easier to manage.

TODO:

pradyunsg · 2017-01-18T03:11:51Z

@dstufft @xavfernandez RFC?

(other than the lack of tests)

Ivoz · 2017-01-18T09:37:44Z

pip/baseparser.py

@@ -132,7 +132,7 @@ def __init__(self, *args, **kwargs):
        self.name = kwargs.pop('name')

        isolated = kwargs.pop("isolated", False)
-        self.config = Configuration(isolated)
+        self._configuration_obj = Configuration(isolated)


We already know it's an object here, by convention (and pretty much everything in python is one), so this naming is redundant.

If you really wish to indicate it's a private member then I'd just rename it _config.

Agreed. It's exactly why I put that there.

I had changed it to prevent collisions if I write that while cracking out the implementation because I don't use _obj suffix either. Forgot to revert that change.

pradyunsg · 2017-01-18T16:14:17Z

Thanks @Ivoz. Any other comments?

pradyunsg · 2017-01-24T17:44:11Z

I'm going to put this on hold for now.

~~Will be working on stuff this weekend.~~

pradyunsg

Just went through this to make some notes.

I don't have much time at hand for this PR in the very near future.

pradyunsg · 2017-02-05T04:44:57Z

pip/commands/configuration.py

+            return ERROR
+
+        # Determine which configuration files are to be loaded
+        if options.user_file and options.global_file:


Check for all 3 (global, venv, user)

pradyunsg · 2017-02-05T04:45:34Z

pip/commands/configuration.py

+        try:
+            self.configuration.save()
+        except Exception:
+            logger.error("Houston, we have a problem", exc_info=1)


Decide how to handle this correctly...

Ask the user to report as bug.

pradyunsg · 2017-02-05T04:45:56Z

pip/configuration.py

+logger = logging.getLogger(__name__)
+
+
+# NOTE: Maybe use the optionx attribtue to normalize keynames.


Good spot me. 😛

pradyunsg · 2017-02-05T04:47:38Z

pip/configuration.py

+def _make_key(variant, name):
+    return ".".join((variant, name))
+
+# Some terminology:


Should this stay separate as a comment or be added to a doc-string?

Maybe, because it involves terminology only for this module - keep it as it is.

Added to docstring.

pradyunsg · 2017-02-05T04:48:10Z

pip/configuration.py

+        if load_only not in ["user", "site-wide", "venv", None]:
+            raise ValueError(
+                "Got invalid value for load_only - should be one of 'user', "
+                "'site-wide', 'venv'"


Include None in the error message.

pradyunsg · 2017-02-05T04:49:04Z

pip/configuration.py

+            section, name = _disassemble_key(key)
+
+            # Remove the key in the parser
+            modified_something = (


Make this less terse? Or have a comment explaining why this works.

It serves the intent. Comments get outdated easily.

This is a bit hard to follow, I generally prefer not to have methods with side effects in a boolean expression because it's easy to miss what's going on. I think this would be more readable as:

modified_something = False if parser_has_section(section): modified_something = parser.remove_option(section, name)

I actually missed that this was having any side effect at first and it took me a minute to understand why you wanted a comment here to start with.

pradyunsg · 2017-02-05T04:51:00Z

pip/commands/__init__.py

@@ -19,6 +20,7 @@

 commands_dict = {


This can be changed into a dict constructor call with a list comprehension... DRY.

Now that we've dropped 2.6 support, it can be a dict comprehension!! :)

pradyunsg · 2017-02-05T04:52:05Z

pip/commands/configuration.py

+
+    def set_name_value(self, options):
+        key, value = options.set_name_value.split("=", 1)
+        self.configuration.set_value(key, value)  # Error here is propagated.


Print a nice message instead of propagating the error? Or is something doing that?

Same on line 212.

pradyunsg · 2017-05-12T10:42:54Z

@dstufft @xavfernandez @pfmoore

I'm writing the tests for this and I am a little confused - how extensive should the tests be and what exact functionalities should be tested?

As such, there are so many things related to configuration that it cannot be verified to be working without modifying the entire environment. The earlier suite only tested precedence and overriding of configuration options. Things like which configuration files would be loaded, the exact contents of the files etc were not tested earlier. I'm thinking they should not be tested now either.

Thoughts?

pfmoore · 2017-05-12T11:01:37Z

@pradyunsg I'd have assumed the best way to test would be

Run a config command
Check that the underlying configuration file was changed in the way you expected

I wouldn't try to test that the behaviour changes as expected. In theory we should already have tests that check "if the config says do X, then we do it", so that side of the problem should already be handled. In practice we likely haven't covered all such cases, but it's not the issue for this PR to address that problem.

pradyunsg · 2017-05-12T11:11:26Z

I am writing those tests right now - that run the command and check the result.

I was wondering about the exact degree that the tests check things... Not checking behaviour changes sounds good to me.

pradyunsg · 2017-05-12T12:19:41Z

The functional tests check the behaviour by doing a subprocess run. This means that if a configuration file has to be loaded, it needs to be in one of the standard locations in order to be used by the test => if I want to check if the user file can be modified using the config command in pip, I will need the modify the actual user file - since I cannot patch in the sub process to change what file it opens. Further, it means that to check if the "site-wide" files can be modified, the test would have to be run escalated and would modify the global file somewhere in /etc/ or something.

This cannot be done as it might involve modifying a file that possibly already exists and contains probably useful contents. And well, escalation to run a test - no.

I'm not able to come up with any way to make the subprocess to use a different path for the test run. I don't want to be touching the default files when running the tests (no modify in-place or move-replace). 😕

pradyunsg · 2017-05-12T12:22:06Z

PS: Travis failed due to some network issues IIUC.

pradyunsg · 2017-05-12T12:24:54Z

@xavfernandez Could you add this to v10.0 milestone? Since #4193 depends on the changes made here, it means that either this also should be a part of v10.0 or that should be removed.

pradyunsg · 2017-05-13T10:19:13Z

FWIW, I just pushed what test code I have. I think the tests can look the same regardless of how the patching is done but I think I'll try to figure out how to patch the files then proceed to actually writing the rest of the tests.

pradyunsg · 2017-05-13T10:20:09Z

I apologize if I'm being noisy.

/ping @dstufft @xavfernandez @pfmoore

dstufft

This looks like it's going in a good direction. I've marked some changes I'd like to see. One that I didn't mark everywhere is not shadowing builtins like file.

dstufft · 2017-05-13T16:16:08Z

pip/baseparser.py

@@ -160,6 +160,9 @@ def _update_defaults(self, defaults):
            # ignore empty values
            if not val:
                continue
+            section, key = key.split(".", 1)
+            if section not in [self.name, "global", ":env:"]:


Small nit, but this can be a set literal now which is slightly nicer.

dstufft · 2017-05-13T16:18:40Z

pip/commands/configuration.py

+        if action is None:
+            logger.error(
+                "Need exactly one action (--list, --edit, "
+                "--get, --set, --unset) to perform."


I feel like it would be better to instead of using a mandatory flag like this, to make this a positional argument to the command. So basically, instead of pip config --list ..., you would do pip config list .... Positional arguments feel like a better fit for mandatory arguments to me.

I agree and it would also prevent the (twisted) corner case where you put:

[config] list = 1

making the use of the config command rather complicated.

Sounds good.

dstufft · 2017-05-13T16:19:40Z

pip/commands/configuration.py

+        if sum([options.user_file, options.global_file, options.venv_file]):
+            logger.error(
+                "Need at-most one configuration file to use - pass "
+                "only one of --global, --user, --venv."


We should probably just default to one of these files. Either the user file, or if we want to be smarter, we could say the venv file if one exists, and otherwise the user file.

The smarter variant sounds better. :)

The only time assuming a file could be useful is when we have to modify something. At the other times - list and get, it makes more sense to be asking the user to specify if they want to load a specific file - in which case we only load that file.

I feel it makes more sense to require the user to pass which file is to be modified or used since it gives them the ability to inspect the effect of each level or overridding as well as a fine degree of control.

dstufft · 2017-05-13T16:22:50Z

pip/commands/configuration.py

+            kwargs["load_only"] = "venv"
+        elif action in ["set", "unset", "edit"]:
+            logger.error(
+                "Need one configuration file to modify - pass one of "


This seems to be duplicate logic to the above since you can't get to this point without one of these files existing. However, my same comment up above applies, I think we should just pick something to default to.

Yep. This'll be redundant.

dstufft · 2017-05-13T16:24:15Z

pip/commands/configuration.py

+        if options.editor is None:
+            logger.error(
+                "--edit requires an editor to be passed, either using "
+                "--editor or by setting it in a configuration file."


I don't see an --editor flag defined up above, did I miss it? It would also be nice to support the semi standard $EDITOR environment variable.

dstufft · 2017-05-13T16:24:58Z

pip/commands/configuration.py

+            )
+            return ERROR
+
+        file = self.configuration.get_file()


Small nit: I prefer not using the name file because it shadows a builtin, file_ is ok, or filename or something like that.

pip/locations.py

xavfernandez

Quite long PR, I'll try to re-review it soon.

xavfernandez · 2017-05-13T16:58:03Z

pip/commands/configuration.py

+        if options.editor is None:
+            logger.error(
+                "--edit requires an editor to be passed, either using "
+                "--editor or by setting it in a configuration file."


I agree with @dstufft, it would make sense to default on the value of $VISUAL or $EDITOR (at least for POSIX systems).

--editor - I think I dropped it by mistake while doing the rebase with master. Oops. I have added it back.

Made the change to check $EDITOR if --editor is not passed. I don't think we need to differ our behaviour across OSs.

xavfernandez · 2017-05-13T17:13:15Z

pip/commands/configuration.py

+            return ERROR
+
+        # Determine which configuration files are to be loaded
+        if sum([options.user_file, options.global_file, options.venv_file]):


Is it missing a >1 or a not or am I missing something ?

Not having it compare with anything is equivalent to checking the falsiness.

Sum of bools gives an integer. All non-zero integers are true-ish.

Anyway, I changed this part.

xavfernandez · 2017-05-13T17:18:07Z

pip/commands/configuration.py

+        if action is None:
+            logger.error(
+                "Need exactly one action (--list, --edit, "
+                "--get, --set, --unset) to perform."


I agree and it would also prevent the (twisted) corner case where you put:

[config] list = 1

making the use of the config command rather complicated.

xavfernandez · 2017-05-13T17:22:22Z

pip/configuration.py

 import os
-import sys
+import logging


nit: alphabetical order ;)

I actually like this increasing length form... :/

Made the change though. :)

xavfernandez · 2017-05-13T17:30:55Z

pip/commands/configuration.py

+            subprocess.check_call([options.editor, file])
+        except subprocess.CalledProcessError as e:
+            logger.error(
+                "Subprocess exited with exit code %d", e.returncode


Maybe also add the editor used in the error msg (so that it is clear that we are not refering to pip, which will also return an error code).

Sounds good.

xavfernandez · 2017-05-13T17:34:26Z

pip/configuration.py

+    def __init__(self, isolated, load_only=None):
+        super(Configuration, self).__init__()
+
+        if load_only not in ["user", "site-wide", "venv", None]:


Since we are using these values in quite a few places, I'd rather we use some constants USER, SITEWIDE, etc.

Umm, I don't feel very strongly about it... I guess.

Will do.

xavfernandez · 2017-05-13T17:41:31Z

pip/configuration.py

+            raise ConfigurationError("Needed a specific file to be modifying.")
+
+    @property
+    def _dictionary(self):


A chainmap would have been nice but the need of a vendored backport in python2.7 would maybe make it a little overkill though.

Yeah... I was thinking about it but decided it was overkill too.

xavfernandez · 2017-05-13T17:44:59Z

pip/commands/configuration.py

+        try:
+            self.configuration.unset_value(key)
+        except ConfigurationError:
+            logger.error("Could not unset value in configuration")


a return ERROR here would be cleaner

xavfernandez · 2017-05-13T17:45:05Z

pip/commands/configuration.py

+        try:
+            self.configuration.set_value(key, value)
+        except ConfigurationError:
+            logger.error("Could not set value in configuration")


a return ERROR here would be cleaner

BrownTruck · 2017-05-14T05:00:22Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

AraHaan · 2017-05-14T05:28:48Z

Will this configuration command also allow setting additional places to download or search for packages that may or may not be on PyPi? Also, it would be nice if the confirmation format was in json in case people want a list of multiple places for pip to look in additionally to PyPi for packages with source or wheel distributions. (Like if they use an github repository to store source distributions / wheels they make in an cental location separate from PyPi for potentially multiple packages.

pradyunsg · 2017-05-14T06:31:37Z

@AraHaan

Will this configuration command also allow setting additional places to download or search for packages that may or may not be on PyPi?

Do you mean adding to a list or removing from it? As of now, it doesn't.
If you're asking if there's some sort of behaviour change in pip, there is no change on the end-user's side.

If that's not what you mean, I'm afraid it's not clear to me what you're asking.

Also, it would be nice if the confirmation format was in json

I think there will be support for TOML based configuration soon (tracking issue: #3809).

pradyunsg · 2017-05-14T06:31:43Z

Do we have options that expect their values to be lists? If so, they'll need special handling, correct?

pradyunsg · 2017-05-14T07:18:50Z

@dstufft @xavfernandez Thanks for the reviews! ^.^

I think I addressed all the review comments. ~~I still have to write a few tests for the command - other than that,~~ is there anything else withstanding?

Should I write more tests for the command?

AraHaan · 2017-05-14T17:13:26Z

The reason why I wanted additional paths to be lists is because they can be easy to append / remove entries on and so then people can do things like specify a specific location in the edit command to add or remove entries on the list. Like for example what if they want pip to look for wheels / source distributions at https://github.com/AraHaan/wheels/blob/master/ without clowning the git repository and a list of other ones from certain other groups doing something similar to other packages.

xavfernandez · 2017-05-14T16:53:10Z

pip/commands/configuration.py

+        # Load a new configuration
+        isolated = options.isolated_mode
+        self.configuration = Configuration(
+            isolated=isolated, load_only=load_only


nit: isolated does not seem very useful

xavfernandez · 2017-05-14T17:06:37Z

tests/functional/test_configuration.py

+"""
+
+import pytest
+import textwrap


nit: standard library imports first, then library

xavfernandez · 2017-05-14T17:06:58Z

tests/lib/configuration_helpers.py

+import textwrap
+import tempfile
+import functools
+import contextlib


import orders 🙄

xavfernandez · 2017-05-14T17:10:05Z

pip/commands/configuration.py

+
+        # Error handling happens here, not in the action-handlers.
+        try:
+            handlers[action](options, args)


args[1:] would make more sense to me: no need to pass args[0] to the handler.

xavfernandez · 2017-05-14T19:51:55Z

pip/configuration.py



-_environ_prefix_re = re.compile(r"^PIP_", re.I)
+def _make_key(variant, name):


Since it is only used in one place and quite simple, I'd say you could ditch it.

xavfernandez · 2017-05-14T19:53:32Z

pip/configuration.py

-        # at the base we have any site-wide configuration
-        files = list(site_config_files)
+        # environment variables have the lowest priority
+        config_file = os.environ.get('kinds.PIP_CONFIG', None)


It looks like a sed gone wrong ^^

xavfernandez · 2017-05-14T19:55:16Z

pip/configuration.py

+        yield kinds.GLOBAL, list(site_config_files)
+
+        # per-kinds.user configuration next
+        kinds.should_load_user_config = not self.isolated and not (


xavfernandez · 2017-05-14T19:55:23Z

pip/configuration.py

+        kinds.should_load_user_config = not self.isolated and not (
+            config_file and os.path.exists(config_file)
+        )
+        if kinds.should_load_user_config:


pradyunsg · 2017-05-15T09:39:47Z

@xavfernandez Thanks for this! ^.^

I had made that change in a hurry and pushed.

pradyunsg · 2017-05-30T18:21:57Z

/request-review @dstufft @xavfernandez @pfmoore

xavfernandez · 2017-05-30T21:10:44Z

pip/baseparser.py

+
+        # Pool the options into different groups
+        section_items = {name: [] for name in override_order}
+        for key, val in self.config.items():


Maybe use for section_key, val in self.config.items(): here and section, key = section_key.split(".", 1) a line below

xavfernandez · 2017-05-30T21:29:10Z

pip/baseparser.py

+        section_items = {name: [] for name in override_order}
+        for key, val in self.config.items():
+            # ignore empty values
+            if not val:


Maybe a logger.debug stating that we are ignoring empty value for such (section, key)

There's no logger in this module. I'll add one. :)

xavfernandez · 2017-05-30T21:41:21Z

pip/commands/configuration.py

+                .format(e.returncode)
+            )
+
+    def _get_n_args(self, args, n):


Maybe add an help/example text, which would allow key, value = self._get_n_args(args, n=2, example="config set timeout 20") in def set_name_value and a nicer error message like:

Got unexpected number of arguments, expected 2 (example: "pip config set timeout 20")

(using pip.utils.get_prog)

xavfernandez · 2017-05-30T21:44:10Z

pip/commands/configuration.py

+        if options.editor is not None:
+            return options.editor
+        elif "EDITOR" in os.environ:
+            return os.environ["EDITOR"]


support for the standard $VISUAL would be nice also (git for example supports both)

xavfernandez · 2017-05-30T21:44:59Z

pip/configuration.py

-from pip.utils import appdirs
+from pip.utils import ensure_dir, enum
+
+_need_file_err_msg = "Needed a specific file to be modifying."


This does not seem to be used

xavfernandez · 2017-05-30T21:51:33Z

pip/configuration.py

-        """Loads configuration
+        # The order here determines the override order.
+        self._override_order = [
+            kinds.GLOBAL, kinds.USER, kinds.VENV, kinds.ENV, kinds.ENV_VAR


A comment explaining the difference between kinds.ENV (the config from the file from PIP_CONFIG_FILE) and kinds.ENV_VAR (the config from environment vars) would have helped :o

I'll add one where kinds is defined.

xavfernandez · 2017-05-30T21:52:50Z

pip/configuration.py

        if not self.isolated:
            self._load_environment_vars()

+    def get_file(self):


get_file_to_edit() maybe ?

Made the change.

xavfernandez · 2017-05-30T21:54:06Z

pip/configuration.py

+        """
+        self._ensure_have_load_only()
+
+        if key not in self._config[self.load_only]:


maybe a logger.debug recalling the value of load_only

Added in _ensure_have_load_only.

xavfernandez · 2017-05-30T21:56:52Z

pip/configuration.py

+            for file_ in files:
+                # If there's specific variant set in `load_only`, load only
+                # that variant, not the others.
+                if self.load_only is not None and variant != self.load_only:


logger.debug ?

xavfernandez · 2017-05-30T21:59:10Z

pip/configuration.py


+    # XXX: This is patched in the tests.
    def _get_config_files(self):


Maybe iter_config_files would be a better name for a generator.

pradyunsg · 2017-06-01T06:22:52Z

Some external change broke the CI builds.

pradyunsg · 2017-06-13T08:38:33Z

Is there anything outstanding on this PR?

dstufft · 2017-06-13T16:02:55Z

pip/configuration.py

+            section, name = _disassemble_key(key)
+
+            # Remove the key in the parser
+            modified_something = (


This is a bit hard to follow, I generally prefer not to have methods with side effects in a boolean expression because it's easy to miss what's going on. I think this would be more readable as:

modified_something = False if parser_has_section(section): modified_something = parser.remove_option(section, name)

I actually missed that this was having any side effect at first and it took me a minute to understand why you wanted a comment here to start with.

pradyunsg changed the title ~~[WIP]Add a configuration command~~ [WIP] Add a configuration command Jan 18, 2017

Ivoz reviewed Jan 18, 2017

View reviewed changes

pradyunsg commented Feb 5, 2017

View reviewed changes

pradyunsg mentioned this pull request Apr 7, 2017

Add the ability to autocorrect a user's command #4193

Closed

pradyunsg force-pushed the configuration-command branch from 57b510d to b76d4e7 Compare April 8, 2017 05:41

dstufft requested changes May 13, 2017

View reviewed changes

xavfernandez reviewed May 13, 2017

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label May 14, 2017

pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 14, 2017

xavfernandez reviewed May 14, 2017

View reviewed changes

pradyunsg changed the title ~~[WIP] Add a configuration command~~ Add a configuration command May 15, 2017

pradyunsg added 4 commits May 20, 2017 12:56

Implement section overriding in baseparser

ac2e6e5

Add a test for environment config file

94d752e

Improve tests

bd850c0

Close the open file-descriptor

19fd2db

pradyunsg force-pushed the configuration-command branch from ac6c367 to 19fd2db Compare May 20, 2017 07:30

xavfernandez reviewed May 30, 2017

View reviewed changes

pradyunsg added 2 commits June 1, 2017 11:25

Address review comments

341766c

Change file_ to fname

3bb4ddc

pradyunsg and others added 3 commits June 13, 2017 10:33

🎨 Misc change to re-trigger CI

8b5cf0d

🔧

d1197d9

Add examples to the sub-commands

b75e7c6

pradyunsg force-pushed the configuration-command branch from dfa1f9a to b75e7c6 Compare June 13, 2017 06:24

pradyunsg added 2 commits June 13, 2017 21:26

Merge branch 'master' into configuration-command

b0ed0f7

Even edit command needs a load-only value

0802d3f

dstufft requested changes Jun 13, 2017

View reviewed changes

pradyunsg added 2 commits June 13, 2017 21:40

Protect get_file_to_edit from not having a load_only

daf754a

Address review comment

b5bfca1

dstufft approved these changes Jun 13, 2017

View reviewed changes

pradyunsg added 2 commits June 13, 2017 22:56

🎨

a7d229d

Sort imports

4e47f04

dstufft merged commit 78573d9 into pypa:master Jun 13, 2017

pradyunsg deleted the configuration-command branch June 13, 2017 19:27

pradyunsg mentioned this pull request Jun 13, 2017

Create a command to make it easy to access the configuration file #1736

Closed

xavfernandez added this to the 10.0 milestone Jun 13, 2017

cjerdonek mentioned this pull request Jan 14, 2019

Option index-url = for specific commands doesn't set index-url to PyPI #6131

Open

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019

		logger = logging.getLogger(__name__)


		# NOTE: Maybe use the optionx attribtue to normalize keynames.



		_environ_prefix_re = re.compile(r"^PIP_", re.I)
		def _make_key(variant, name):


		# XXX: This is patched in the tests.
		def _get_config_files(self):

Add a configuration command #4240

Add a configuration command #4240

Conversation

pradyunsg commented Jan 18, 2017 • edited Loading

pradyunsg commented Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Jan 18, 2017

pradyunsg commented Jan 24, 2017 • edited Loading

pradyunsg left a comment • 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

pradyunsg Apr 8, 2017 • 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

pradyunsg commented May 12, 2017

pfmoore commented May 12, 2017

pradyunsg commented May 12, 2017 • edited Loading

pradyunsg commented May 12, 2017 • edited Loading

pradyunsg commented May 12, 2017

pradyunsg commented May 12, 2017

pradyunsg commented May 13, 2017

pradyunsg commented May 13, 2017

dstufft 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

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

xavfernandez 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrownTruck commented May 14, 2017

AraHaan commented May 14, 2017 • edited Loading

pradyunsg commented May 14, 2017

pradyunsg commented May 14, 2017

pradyunsg commented May 14, 2017 • edited Loading

AraHaan commented May 14, 2017

Choose a reason for hiding this comment

pradyunsg commented Jan 18, 2017 •

edited

Loading

pradyunsg commented Jan 18, 2017 •

edited

Loading

pradyunsg commented Jan 24, 2017 •

edited

Loading

pradyunsg left a comment •

edited

Loading

pradyunsg Apr 8, 2017 •

edited

Loading

pradyunsg commented May 12, 2017 •

edited

Loading

pradyunsg commented May 12, 2017 •

edited

Loading

AraHaan commented May 14, 2017 •

edited

Loading

pradyunsg commented May 14, 2017 •

edited

Loading