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

minor refactors/pep8tification #414

Merged
merged 8 commits into from Jun 12, 2017

Conversation

Projects
None yet
3 participants
@pravinkc
Contributor

pravinkc commented Jun 4, 2017

I was going thru the codebase and added some refactors/comments etc.

@bowlofeggs

Not being at all familiar with the codebase, it seems reasonable to me.

@@ -163,6 +164,7 @@ def __init__(self, **config):
pass
# Cleanup. See https://bit.ly/SaGeOr for discussion.
# arg signature - weakref.ref(object [, callback])

This comment has been minimized.

@bowlofeggs

bowlofeggs Jun 6, 2017

Member

Can you explain this new line?

This comment has been minimized.

@pravinkc

pravinkc Jun 7, 2017

Contributor

I just put the argument signature here, in case someone doesn't know about it (l had to look it up for example)

@@ -369,7 +372,7 @@ def _create_poller(self, topic="", passive=False, **kw):
# TODO -- the 'passive' here and the 'active' are ambiguous. They
# don't actually mean the same thing. This should be resolved.
method = passive and 'bind' or 'connect'
method = (passive and 'bind') or 'connect'

This comment has been minimized.

@bowlofeggs

bowlofeggs Jun 6, 2017

Member

This change doesn't seem necessary, though I also don't think it is harmful. The order of operations already works this way.

This comment has been minimized.

@pravinkc

pravinkc Jun 7, 2017

Contributor

I think this makes it more readable. Also, from the zen of python "Explicit is better than implicit"

@@ -463,7 +467,7 @@ def _run_socket(self, sock, name, ep, watched_names=None):
else:
raise ValidationError(msg)
else:
return name, ep, _topic, msg
return (name, ep, _topic, msg)

This comment has been minimized.

@bowlofeggs

bowlofeggs Jun 6, 2017

Member

This change doesn't seem necessary - it's functionally equivalent.

This comment has been minimized.

@pravinkc

pravinkc Jun 7, 2017

Contributor

equivalent true, it's just more explicit this way i think?
i can remove it (and other things you mention 🔝 if required 👍)

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

Personally I see them as equivalent in explicitness, but it's not surprising to run across this and it doesn't hurt anything so I don't mind one way or the other.

As a side node, something I think is more explicit and something I quite like in this sort of scenario is a namedtuple. However I think that's probably better as a separate PR.

@@ -62,7 +62,7 @@ def __init__(self, **config):
self.log = logging.getLogger("fedmsg")
self.c = config
self.hostname = socket.gethostname().split('.', 1)[0]
self.hostname = socket.gethostname().split('.', maxsplit=1)[0]

This comment has been minimized.

@jeremycline

jeremycline Jun 7, 2017

Member

maxsplit is a positional argument in Python 2.7, which we unfortunately still have to support.

This comment has been minimized.

@pravinkc

pravinkc Jun 7, 2017

Contributor

great catch, fixed!

@bowlofeggs

This comment has been minimized.

Member

bowlofeggs commented Jun 7, 2017

@jeremycline

This comment has been minimized.

Member

jeremycline commented Jun 7, 2017

CI still seems to be unhappy:

FAIL: send_message is deprecated
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/fedora-infra/fedmsg/fedmsg/tests/test_core.py", line 29, in test_send_message
    assert str(w[0].message) == ".send_message is deprecated."
AssertionError:
    <module 'warnings' from '/home/travis/virtualenv/python2.6.9/lib/python2.6/warnings.pyc'>.simplefilter("always")
    self.ctx.send_message(topic='org.fedoraproject.prod.compose.rawhide.complete', msg="{'arch'': 's390', 'branch': 'rawhide', 'log': 'done'}")
    assert len([<warnings.WarningMessage object at 0x254d650>]) == 1
>>  assert str([<warnings.WarningMessage object at 0x254d650>][0].message) == ".send_message is deprecated."
    assert self.ctx.publish.called
    topic, msg, modname = self.ctx.publish.call_args[0]

Can you look into that?

Note that the issue about importing OrderedDict on Python 2.6 has been fixed in mokshaproject/moksha#41 and just needs a Moksha release before it's good to go.

@pravinkc pravinkc force-pushed the pravinkc:minor_refactors branch 3 times, most recently from 5e3184e to 0f1e80d Jun 8, 2017

@pravinkc pravinkc force-pushed the pravinkc:minor_refactors branch 2 times, most recently from 637bf52 to 03f6b84 Jun 8, 2017

@pravinkc pravinkc force-pushed the pravinkc:minor_refactors branch from 03f6b84 to 4f34934 Jun 8, 2017

@pravinkc

This comment has been minimized.

Contributor

pravinkc commented Jun 8, 2017

@jeremycline I have fixed the tests and went on to add flake8 to the test suite.
only py26 fails because now because of moksha as you mentioned 🔝

@jeremycline

Thanks so much for this, it's really great.

I have a few notes inline, but I'm excited for this!

.travis.yml Outdated
- pip install --upgrade pip setuptools
- pip install -r test-requirements.txt
- pip install -e .[commands,consumers]
- pip install codecov

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

I've actually got this going in a separate PR (#417), but I like where your head is at 😄

@@ -75,7 +75,7 @@ def __init__(self, **config):
module_name = guess_calling_module(default="fedmsg")
config["name"] = module_name + '.' + self.hostname
if any(map(config["name"].startswith, ['fedmsg'])):

This comment has been minimized.

@jeremycline
@@ -139,7 +139,8 @@ def __init__(self, **config):
# If we fail to bind or connect, there's probably another
# process already using that endpoint port. Try the next
# one.
pass
self.log.debug(

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

Technically this isn't a PEP-8 change or minor refactor, so although I'd definitely accept a change like this, I'd prefer it to be in a separate PR

@@ -379,6 +382,7 @@ def _create_poller(self, topic="", passive=False, **kw):
# it appears in the endpoints list due to a hack where it gets
# added in __init__ above.
if _name == 'relay_inbound':
self.log.debug("ignored relay_inbound from config.endpoints")

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

Same deal as above. I'd definitely accept a PR that improves the logging, but it's not really related to the other stuff in this PR.

@@ -463,7 +467,7 @@ def _run_socket(self, sock, name, ep, watched_names=None):
else:
raise ValidationError(msg)
else:
return name, ep, _topic, msg
return (name, ep, _topic, msg)

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

Personally I see them as equivalent in explicitness, but it's not surprising to run across this and it doesn't hurt anything so I don't mind one way or the other.

As a side node, something I think is more explicit and something I quite like in this sort of scenario is a namedtuple. However I think that's probably better as a separate PR.

import M2Crypto
import m2ext
imp.find_module('M2Crypto')
imp.find_module('m2ext')

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

Same note about imp as above 😄

@@ -74,7 +75,7 @@ def skip_if_fedmsg_meta_FI_is_present(f):
"""
def _wrapper(self, *args, **kw):
try:
import fedmsg_meta_fedora_infrastructure
imp.find_module('fedmsg_meta_fedora_infrastructure')

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

Same note as above about imp

@@ -61,7 +63,7 @@ def scan_all():
info()
if 'verbose' in sys.argv:
import pprint;
import pprint

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

Could you move this import to the top of the file?

tox.ini Outdated
@@ -1,7 +1,11 @@
[tox]
envlist = py{27}-six{19,Latest}
envlist = py26,py27-six19,py27-sixLatest,py33,py34,py35,flake8

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

I'd prefer changes to the tox file to occur in a separate PR (and I've got a lot of this in #417 already 😃 )

@@ -9,3 +9,6 @@ ordereddict; python_version <= '2.6'
logutils; python_version <= '2.6'
unittest2; python_version <= '2.6'
Twisted==12.2; python_version <= '2.6'
# flake8 3 dropped support for py{26,33}

This comment has been minimized.

@jeremycline

jeremycline Jun 9, 2017

Member

I'm fine only running flake8 on 2.7 since it's for developers and we should all have at least 2.7

pravinkc added some commits Jun 10, 2017

Merge branch 'develop' of https://github.com/fedora-infra/fedmsg into…
… minor_refactors

* 'develop' of https://github.com/fedora-infra/fedmsg:
  Add a fedmsg-check command
  Switch to the pytest test runner
  Set Travis CI to run via tox
  Add a signing relay
@pravinkc

This comment has been minimized.

Contributor

pravinkc commented Jun 10, 2017

I have made the requested changes, @jeremycline 👍

@jeremycline

This comment has been minimized.

Member

jeremycline commented Jun 12, 2017

Great, thanks so much!

:shipit:

@jeremycline jeremycline merged commit f0553d2 into fedora-infra:develop Jun 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sijis added a commit to sijis/fedmsg that referenced this pull request Jan 8, 2018

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