Project boilerplate #3

Merged
merged 10 commits into from Aug 19, 2015

Conversation

Projects
None yet
5 participants
Contributor

huwshimi commented Aug 14, 2015

Here is a bunch of project setup stuff. Added setup.py, Makefile, requirements, lint etc.

test-requirements.txt
+flake8==2.4.1
+mccabe==0.3.1
+mock==1.3.0
+nose==1.3.7
@mitechie

mitechie Aug 17, 2015

Collaborator

we shouldn't need nose and pytest. I think talking with @markramm the thought is to stick to pytest.

@markramm

markramm Aug 17, 2015

Owner

I think just using pytest should be enough -- is there something in Nose that we need?

Makefile
+ $(PIP) install -r requirements.txt
+
+# Nose is our test requirements canary
+$(NOSETESTS): $(PYTHON) deps
@mitechie

mitechie Aug 17, 2015

Collaborator

so this should turn into pytest for test runs and checks.

@bac

bac Aug 17, 2015

Regarding @mitechie's comment earlier, since deps is a phony (not a real target on the file system with a timestamp), running these tests will cause the deps target to fire every time.

So deps and test-deps should probably have canary files. Something like:

https://pastebin.canonical.com/137717/

The goal is that invoking a makefile target more than once results in nothing being done on subsequent tries. If you run 'make deps' repeatedly now you'll see it invokes pip install each time. And it'll do it every time you try to run tests.

Collaborator

mitechie commented Aug 17, 2015

@markramm @howbazaar can you do a second review of this please? I've asked Huw to look at some project skeleton work to make sure it can be built into a python package and that there's the bones to hang common tools like test running, linting, etc off of.

Makefile
+# DEPENDENCIES
+##############
+.PHONY: test-deps
+test-deps: $(PIP)
@mitechie

mitechie Aug 17, 2015

Collaborator

check with @bac as I think his trick with working with deps files has been a great way to speed things up. His rework on our own storefront app has implemented this for comparison.

@bac

bac Aug 17, 2015

This should probably depend on a canary file and on test-requirements.txt. After the install be sure to touch the canary.

+.PHONY: check
+check: clean-all lint test
+
+###############
@mitechie

mitechie Aug 17, 2015

Collaborator

there should also be a releases section that will build the pypi release and we should also generate a wheel as we do on other things.

setup.py
+
+setup(
+ name='jujulib',
+ version='0.1.0',
@mitechie

mitechie Aug 17, 2015

Collaborator

let's start with 0.0.1 while in the early stages please.

Collaborator

mitechie commented Aug 17, 2015

Thanks @huwshimi, a few other requests/changes please.

Owner

markramm commented Aug 17, 2015

+1 with Rick's changes.

Makefile
+ $(PIP) install -r test-requirements.txt
+
+.PHONY: deps
+deps: $(PIP)
@bac

bac Aug 17, 2015

Likewise add requirements.txt to this dependency so the deps will get regenerated when that file changes.

Makefile
+$(NOSETESTS): $(PYTHON) deps
+ $(MAKE) test-deps
+
+$(FLAKE8): $(NOSETESTS)
@bac

bac Aug 17, 2015

I don't understand this rule.

Makefile
+ENV := env/
+BIN := $(ENV)bin/
+FLAKE8 := $(BIN)flake8
+NOSETESTS := $(BIN)nosetests
@bac

bac Aug 17, 2015

It's trivial, but it is just weird having the trailing slash in the BIN definition because this now looks funny.

Makefile
+
+# Nose is our test requirements canary
+$(NOSETESTS): $(PYTHON) deps
+ $(MAKE) test-deps
@bac

bac Aug 17, 2015

This target is not correct as it just just runs 'make test-deps' which is already a dependency.

bac commented Aug 17, 2015

Thanks for this branch @huwshimi. I got kind of carried away looking at the Makefile to ensure I wasn't telling you something dumb. Here is a version that I think is pretty good:

https://pastebin.canonical.com/137718/

With these and @mitechie's suggestions 👍

Contributor

huwshimi commented Aug 18, 2015

I have made these changes with the exception of "there should also be a releases section that will build the pypi release and we should also generate a wheel as we do on other things." which I am doing in a follow up branch so as to not make this too big.

+
+.PHONY: clean-env
+clean-env:
+ rm -rf $(ENV)
@bac

bac Aug 18, 2015

This target should also remove the canaries. Sorry I neglected it.

bac commented Aug 18, 2015

👍 with one small change.

Collaborator

mitechie commented Aug 18, 2015

👍 with @bac's note. Thanks for the update!

howbazaar added a commit that referenced this pull request Aug 19, 2015

@howbazaar howbazaar merged commit f66d296 into markramm:master Aug 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment