Project Feedback

[bradmontgomery]

This is essentially the same question and feedback that I gave at https://github.com/HumanDynamics/openPDS-RegistryServer/issues/18, with a couple of minor differences.

I've recently pulled down this project and attempted to get it (with openPDS-RegistryServer) running. A couple of things struck me as surprising and/or unexpected:

• It's unusual for a django project to be distributed (i.e. with a whole settings.py file, ). Typically I'd expect a django app or a collection of apps.
• Your installation instructions tightly couple the project with a virtualenv. These two things should be separated. I should be able to completely remove a virtualenv (or switch to another one) completely separate from my django project.
• There appear to be a few tests, but no information on how to run them.
• The included setup.py, which a neat idea is not what I expect from a setup.py (e.g. a setuptools installation script). Perhaps this should be renamed to something like project_bootstrap.py?

A few things I'd typically expect from a reusable django app are:

• Documentation for any app-specific settings (not a settings file)
• A requirements.txt in the root of the project. This project has this in conf/, but the # XXX - comments don't instill a lot of confidence. (e.g. does this project actually need/use everything in the requirements file?)
• A setuptools-compatible setup.py that allowed me to install with: pip install openPDS
• Some level of tests, runnable with: python manage.py test openpds or instructions for running the test suite in the readme or the docs.

If the goal of this project is to be something that fits within the django ecosystem, the barrier to usage and contributions would be lower if it followed some of the typical community conventions. Some of this information is now available in the django docs, as well.

I hope this is helpful feedback!

[brian717]

I'm going to go ahead and reference https://github.com/HumanDynamics/openPDS-RegistryServer/issues/18 here, since my reply there covers most of this as well.

For the parts that are specific to this issue:

• setup.py can be renamed to avoid confusion. Probably not project_bootstrap.py, but we'll think of something appropriate.

The comments Re: Django from my other reply apply here as well.

I'll go ahead and file an issue to rename setup.py now. Thanks again for the detailed feedback!

[bradmontgomery]

Yes, I don't like project_bootstrap.py either. Thanks for the update!

[RogerTangos]

How's start.py?

And regarding requirements.txt, is the convention to be in the root of the project, instead of in the conf directory?

[bradmontgomery]

How's start.py

That sounds good to me!

And regarding requirements.txt, is the convention to be in the root of the project, instead of in the conf directory?

For a typical django app, I'd expect something like this:

django-myapp/
    myapp/
    README
    setup.py
    requirements.txt

or even a requirements directory (sometimes with different requirements for using an app vs. contributing to an app). Honestly, though, it doesn't bother me that much that yours are in conf, as long as the docs are clear about where to find them.

The thing that does concern me are the unpinned requirements (e.g. what version of requests do you expect) and the # XXX comments. It wouldn't really be good to install libraries that aren't needed. E.g. You could have separate requirements files for someone that wanted to run on top of Postgres v.s. Mongo v.s. SQLite

[RogerTangos]

cool. I'm moving requirements out, because might as well.

It's significantly harder to generate the requirements.txt and then get the project to build. You might note the number of times psycopg2 and pymongo are imported throughout the project...

I'm filing it as an enhancement, and suppose we could get around that by just importing a database_reqs file, and generating that from template.

Regarding requirements, is it preferable to have version number specified in all cases?

[bradmontgomery]

Regarding requirements, is it preferable to have version number specified in all cases?

I personally think so. That way newcomers to the project get the same dependencies. Of course, as the project matures, it's ok to upgrade dependency versions, too.

:+1: on everything else.

[RogerTangos]

cool beans. Thanks again for the feedback. I'm personally quite glad to see unaffiliated people taking a look at and building the code . (It wasn't so easy before Brian's heroic https://github.com/HumanDynamics/openPDS/commit/da408f3de451dea28d1ae7d15a0cec328d0cb3e3)

[bradmontgomery]

Wow, that's quite a commit! :)

You're welcome. This looks like a cool project and I'd like to help. I'd like to start contributing some tests next, which will 1) help me understand the code and 2) prevent me from breaking things moving forward.

[brian717]

Re: references to pymongo... these (except for the mongo backend implementation) can be removed now that backends are fully configurable. An improvement can be filed to clean these up.

References to psycopg2.... I don't think there are any besides postgresql.py (the backend implementation), which is necessary.

So.... I believe we can safely remove the psycopg2 dependency. If you choose psycopg2 as a backend, you'll obviously have to install it, but I think that's expected, and will become obvious with the "psycopg2 not found" error message that would result. I went ahead and did that just now in the dev branch (eb4e2731e03510c34829f65a8e25eef3dc7ea2a5) .

As for removing the mongo dependency for folks that aren't interested in using it - that can't be done until we've cleaned up the unnecessary pymongo imports (I believe the only ones that are still in use are in old answer modules that used mongo directly). This will take a bit more time, so I filed an improvement for this.

[RogerTangos]

Yes Please! One of the reasons that we haven't done tests yet is that I'm not familiar with django testing libraries. A few examples would definitely put is in the right direction.

About those tests -- we're going to be removing the visualizations from the dev branch of openPDS , so you may want to avoid testing them. Other things (like posting data to the connectors) and database connections are well worth testing.

Albert Carter

Programmer Big Data Initiative CSAIL, MIT

On Tue, Nov 4, 2014 at 5:38 PM, Brad Montgomery notifications@github.com wrote:

Wow, that's quite a commit! :)

You're welcome. This looks like a cool project and I'd like to help. I'd like to start contributing some tests next, which will 1) help me understand the code and 2) prevent me from breaking things moving forward.

— Reply to this email directly or view it on GitHub https://github.com/HumanDynamics/openPDS/issues/22#issuecomment-61728258 .