archivesspace-labs / ArchivesSnake

A client library for working with the ArchivesSpace API
https://archivesspace-labs.github.io/ArchivesSnake/
Other
78 stars 13 forks source link

ArchivesSnake does not play well with Pylint #11

Closed AustinTSchaffer closed 6 years ago

AustinTSchaffer commented 6 years ago

The way that the HTTP methods are generated in the meta-class is certainly very interesting, but it does not interface well with Pylint, at least not in my current configuration.

I don't see anything in the docs about it. I'll gladly submit a pull request to manually write out all of the HTTP methods, unless it was decided that generating these methods at runtime is the best solution.

Configuration

$ pylint --version
No config file found, using default configuration
pylint 1.8.4,
astroid 1.6.3
Python 3.5.3 (default, Jan 19 2017, 14:11:04)
[GCC 6.3.0 20170118]

HTTP Method Setup in MetaClass

https://github.com/archivesspace-labs/ArchivesSnake/blob/d876cd3b9154378c78cd41a3ea018d4f48aac15b/asnake/client/web_client.py#L30

Pylint Error

image

pobocks commented 6 years ago

So, I don't know if this is sufficient to fix your personal setup, but pylint does have a "generated-members" option, which can be passed a regexp of methods to omit from checking bc generated.

In general, I'm personally not disposed to fit code mechanisms (as opposed to styles) to particular tooling rather than the other way around, so I'd probably vote against a PR for changing this, but if there's a config file we could include to assauge pylint, that might be a good addition!

AustinTSchaffer commented 6 years ago

@pobocks

I played around with the generated-members Pylint setting that you mentioned last night as well as the ignored-classes setting. I was able to get the E1101 errors to disappear using a .pylintrc on the root of my workspace directory, but all of the methods that I tried had some sort of side affect.

1. False Negatives When Using ignored-classes

I have managed to get Pylint to ignore static checking on the whole ASnakeClient class, using ignored-classes=ASnakeClient, but that disables all E1101 errors against the ASnakeClient class, not just the ones related to the runtime creation of HTTP methods.

image

2. False Negatives When Using generated-members

I have managed to get Pylint to ignore static checking all references to HTTP methods using generated-members=get,post,delete,etc, but that disables all references to those keywords. Looking through other issues in the Pylint repository, I'm not seeing a way to specify the class. You have to specify the variable name for generated members.

image

3. True Positive When Specifying Variable Name

I managed to get Pylint to ignore static checking on references to HTTP methods when I specified the variable name that holds an instance of the class, but it does not appear that you can specify the class and have it apply to the variable. I would consider this as a workaround, but I'd prefer to not have to setup a .pylintrc every time I start a new migration project.

image

Additionally, I'm not seeing a way to specify those settings in a .pylintrc within the asnake package. Pylint checks upwards for configuration files, so those settings would only affect Pylint's operation when checking files that exist under the directory where the configuration file exists.

Are you sure that you would prefer to keep the runtime generation of HTTP methods? I'm on board with preserving function over form, but I feel that this is more than just a code style consideration.

pobocks commented 6 years ago

That's a shame.

I'll ask around, maybe I'll get outvoted, but I feel like the metaprogramming there is worthwhile - it makes it clear that those methods are strict proxies to the requests methods, prevents errors related to changing code in one and not the others. Sphinx can pick it up, and the "generated-members" option makes me think that this is understood by the pylint devs to be a legitimate thing to do, even though pylint's implementation isn't able to be configured for a reasonable workflow around it.

AustinTSchaffer commented 6 years ago

Sorry about that. Thanks for asking around. I'll put in a pull request to manually stub out those methods in the meantime.

The biggest issue with Pylint here is that it only does static code analysis. I think that the dynamic method addition is a legitimate solution in some circumstances, since it works. In this situation, you already know the names and the docstrings of the methods that you want to implement, so I don't see a reason why they can't be explicitly set.

Thank you for your time on this. By the way, how did you generate the documentation for those methods? (https://archivesspace-labs.github.io/ArchivesSnake/html/index.html#asnake.client.ASnakeClient)

pobocks commented 6 years ago

There's nothing in principle preventing it, it's that I think there's a positive value to the existing implementation - reducing duplication and making the proxy nature of these methods explicitly clear. I wouldn't bother with the PR - some people on slack got back to me and are also negative on changing it.

I'll find where in our Sphinx config I set it up to do that - I remember it being some work, but not outside normal configuration.

pobocks commented 6 years ago

:inherited-members: in autodoc pulls them in IIRC, on my phone right now, but it's in [docs/index.rst], if you need more context, I'd be happy to find the relevant Sphinx docs for you later!