Skip to content
This repository was archived by the owner on Jan 18, 2025. It is now read-only.

Conversation

@theacodes
Copy link
Contributor

Fixes #352

@theacodes
Copy link
Contributor Author

Most of this is just moving code around, however, I did have to surgically remove the multstore tests from the file tests.

@dhermes
Copy link
Contributor

dhermes commented Dec 4, 2015

@jonparrott Please please please please please stop doing "just a little extra" in a PR.

Do the surgery first. Then do this renaming.

@dhermes
Copy link
Contributor

dhermes commented Dec 4, 2015

Such ocean. Much boil.

@theacodes
Copy link
Contributor Author

I'm sorry, I know these take lots of time to review. The options were:

  • Make tests.test_file depend on a contrib module, and it being the only core test or module that does.
  • Move the tests into tests.contrib.test_multistore_file, therefore making clean separation between core and contrib.

I really don't like to leave the codebase in a intermediate state such as "moved to contrib, but core still depends on it".

This is the last boil the ocean commit. Promise. I am planning to follow through with renaming some of the older storage classes, but I'll do each of those as a separate commit.

@dhermes
Copy link
Contributor

dhermes commented Dec 4, 2015

Ha, last one! Bold statement.

Why is splitting the tests apart not doable as a pre-amble to this commit? (UPDATE: i.e. Why does that leave things in a broken state?)

@theacodes
Copy link
Contributor Author

Why is splitting the tests apart not doable as a pre-amble to this commit?

You mean split the tests first, then do this? I mean, I could. If you want.

@dhermes
Copy link
Contributor

dhermes commented Dec 4, 2015

You should, I do.

This PR should just be a bunch of file renames.

@theacodes
Copy link
Contributor Author

Will do. Would you prefer two PRs or two commits?

(This PR will need to change imports and a few mock statements, so it won't be as simple as just file renames)

@dhermes
Copy link
Contributor

dhermes commented Dec 4, 2015

Two PRs.

@dhermes
Copy link
Contributor

dhermes commented Dec 4, 2015

Yes I realize it's not as simple as file renames, but the other changes (like ignoring new dirs on tox.ini) are simple enough to understand.

@theacodes
Copy link
Contributor Author

@dhermes this has been rebased and is ready for review.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Dec 11, 2015

Finished review. Looks mostly good. Ping me once done with changes (I asked for at least one, but maybe only one, if that's the case I apologize for making changes plural).

@dhermes
Copy link
Contributor

dhermes commented Dec 14, 2015

LGTM

dhermes added a commit that referenced this pull request Dec 14, 2015
Move non-core modules to contrib.
@dhermes dhermes merged commit 962ad5d into googleapis:master Dec 14, 2015
@dhermes
Copy link
Contributor

dhermes commented Dec 15, 2015

@jonparrott Did you mean to erase the appengine module in this PR?

@dhermes
Copy link
Contributor

dhermes commented Dec 15, 2015

NVM I am stupid

@dhermes
Copy link
Contributor

dhermes commented Dec 15, 2015

======================================================================
ERROR: Failure: ImportError (No module named dev_appserver)   
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../oauth2client/.tox/cover/local/lib/python2.7/site-packages/nose/loader.py", line 418, in loadTestsFromName
    addr.filename, addr.module)
  File ".../oauth2client/.tox/cover/local/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File ".../oauth2client/.tox/cover/local/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File ".../oauth2client/tests/contrib/test_appengine.py", line 32, in <module>
    import dev_appserver
ImportError: No module named dev_appserver
-------------------- >> begin captured logging << --------------------
py.warnings: WARNING: .../oauth2client/.tox/cover/local/lib/python2.7/site-packages/django/db/models/fields/subclassing.py:22: RemovedInDjango110Warning: SubfieldBase has been 
deprecated. Use Field.from_db_value instead.
  RemovedInDjango110Warning)

@dhermes
Copy link
Contributor

dhermes commented Dec 15, 2015

OK maybe I'm not crazy since

nosetests --with-coverage --cover-package=oauth2client --cover-package=tests \
--cover-erase --cover-tests --cover-branches --ignore-files=contrib/test_appengine\.py

should not result in an import error in

.../oauth2client/tests/contrib/test_appengine.py

@theacodes
Copy link
Contributor Author

Ugh, app engine. Created a new PR #363.

@dhermes
Copy link
Contributor

dhermes commented Dec 15, 2015

Thanks. AFAICT the issue was actually the slash that we discussed.

@dhermes dhermes mentioned this pull request Jan 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants