Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Features/testing dry hack #11

Closed

Conversation

guyzmo
Copy link
Contributor

@guyzmo guyzmo commented May 4, 2017

just a simple illustration of #10.

it's not perfect: the name_id properties had to be changed to id and the hook_type had to be renamed type. That's not great regarding shadowing of globals, but it's ok because in usage they'll always be namespaced.

@guyzmo
Copy link
Contributor Author

guyzmo commented May 4, 2017

(I might not have time to finish this, but at least my goal was to give you some ideas about that, and open a discussion)

@ethantkoenig
Copy link
Contributor

ethantkoenig commented May 8, 2017

I'm intrigued, but could you rebase this onto master? It contains some changes (namely 6ae13ca) that are already on master.

@guyzmo guyzmo force-pushed the features/testing_DRY_hack branch from d7da703 to 26a2d6a Compare May 8, 2017 15:07
@guyzmo guyzmo changed the title [WIP] Features/testing dry hack Features/testing dry hack May 8, 2017
@guyzmo
Copy link
Contributor Author

guyzmo commented May 8, 2017

ok, rebased and actually finished it.

@guyzmo
Copy link
Contributor Author

guyzmo commented May 8, 2017

I also updated the travis test to use pytest with a proper coverage report. Hope you'll like it!

@guyzmo
Copy link
Contributor Author

guyzmo commented May 8, 2017

collected 35 items

tests/http_utils_test.py ......
tests/interface_test.py .............................

----------- coverage: platform linux, python 3.6.1-final-0 -----------
Name                                        Stmts   Miss  Cover   Missing
-------------------------------------------------------------------------
gogs_client/__init__.py                         4      0   100%
gogs_client/_implementation/__init__.py         0      0   100%
gogs_client/_implementation/http_utils.py      26      1    96%   36
gogs_client/auth.py                            30      2    93%   20, 61
gogs_client/entities.py                        84      1    99%   12
gogs_client/interface.py                      202     22    89%   110, 231-242, 378, 410, 612-613, 620-6
21, 628-629, 636-637, 644-645, 679, 707, 710, 719
gogs_client/updates.py                         82      6    93%   68-69, 108-109, 151-152
-------------------------------------------------------------------------
TOTAL                                         428     32    93%

@guyzmo guyzmo force-pushed the features/testing_DRY_hack branch 6 times, most recently from 647f9ef to 3c2cddc Compare May 8, 2017 15:37
@guyzmo
Copy link
Contributor Author

guyzmo commented May 8, 2017

sorry for the build flood, but I had to make a bunch of minor adjustments to have it all work 👌 on travis 😉

@@ -585,7 +585,7 @@ def check_for_basic_auth(self, request):
self.assertEqual(request.headers["Authorization"], "Basic {}".format(b64))

def assert_repos_equal(self, repo, expected):
self.assertEqual(repo.repo_id, expected.repo_id)
self.assertEqual(repo.id, expected.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a repo_id property to GogsRepo? I'm fine with having an id property, but I'd like to keep repo_id to maintain backwards compatibility. (similarly for everything else renamed from XYZ_id to id)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👌

.travis.yml Outdated
@@ -5,11 +5,13 @@ python:
- '3.4'
- '3.5'
install:
- pip install -r requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to .travis.yml should be a separate PR; from what I can tell this isn't related to the DRY refactor in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👌

@guyzmo guyzmo force-pushed the features/testing_DRY_hack branch from 3c2cddc to e955663 Compare May 8, 2017 18:31
@guyzmo
Copy link
Contributor Author

guyzmo commented May 8, 2017

otherwise, how do you feel about this change?

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Coverage decreased (-1.3%) to 92.661% when pulling e955663 on guyzmo:features/testing_DRY_hack into aea8d9d on unfoldingWord-dev:master.

@ethantkoenig
Copy link
Contributor

ethantkoenig commented May 8, 2017

@guyzmo Unfornuately, the refactor messed up the docs (we autogenerate Sphinx documentation from docstrings). Essentially, we should be using comment of the form

#: Description
#:
#: :rtype: str

instead of docstrings for class attributes. Also, we need to update entities.rst to be like

.. autoclass:: gogs_client.entities::GogsUser()

    .. autoattribute:: id
        :annotation:

    .. autoattribute:: username
        :annotation:

    .. autoattribute:: email
        :annotation:

    .. autoattribute:: full_name
        :annotation:

    .. autoattribute:: avatar_url
        :annotation:

instead of

.. autoclass:: gogs_client.entities::GogsUser()
    :members:

I've started making these changes at 57f51cd. If you can finish it off, that would be great; otherwise I can finish sometime later this week.

@guyzmo
Copy link
Contributor Author

guyzmo commented May 9, 2017

so, I have took another path: keeping members and keep the documentation close to where the attribute is.

There's also an issue for attrs to generate ivar values appropriately python-attrs/attrs#104 for a class, so it does not build the awful Attribute(……………) crap!

As my current commit is, the only remaining issue is the repetition of the subclasses like GogsRepo.Urls etc.

@ethantkoenig
Copy link
Contributor

+1 for the changes to conf.py, but why did you change the format of all the docstrings? I would prefer using :type: (had previously been using :rtype: but :type: probably makes more sense) instead of (as *int*)

@ethantkoenig
Copy link
Contributor

I've manually merged e955663 and some extra changes I made into master, so I'm closing this PR.

@guyzmo
Copy link
Contributor Author

guyzmo commented May 9, 2017

👍 happy you liked the idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants