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

Configuration cli argument for document_consumer #313

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ovv
Contributor

ovv commented Feb 24, 2018

See #305

Putting this out to get early feedback

@danielquinn

It's hard to find any complaints with this so far, but I do try to be a stickler for style, so I had to include a nitpick.

Aside from that, please make sure that your final PR conforms to pep8 (Travis will complain at you). But otherwise, this is great stuff!

def handle(self, *args, **options):
self.verbosity = options["verbosity"]
directory = options['directory']

This comment has been minimized.

@danielquinn

danielquinn Feb 25, 2018

Owner

Please stick to using " for strings unless those strings contain a " themselves

@danielquinn

This comment has been minimized.

Owner

danielquinn commented Feb 26, 2018

Hey @ovv, just a heads-up that I merged a change this morning that moves pytest.ini to setup.cfg (along with a slew of other things like Coveralls support). As a result, your branch now has a conflict, but if you stash your changes & rebase off master, you'll only need to re-remove that one line from setup.cfg.

@ovv ovv changed the title from WIP: Configuration cli argument for document_consumer to Configuration cli argument for document_consumer Feb 26, 2018

@ovv

This comment has been minimized.

Contributor

ovv commented Feb 26, 2018

This should be ok for review @danielquinn

I am a bit confused why you decided to remove tox. It's quite an useful tool for testing on multiple python version without waiting on Travis. It should handle out of the box settings in setup.cfg.

@@ -80,6 +80,13 @@ you'll need to have it start in the background -- something you'll need to
figure out for your own system. To get you started though, there are Systemd
service files in the ``scripts`` directory.
Some command line argument are available to customize the behavior of the

This comment has been minimized.

@danielquinn

danielquinn Feb 27, 2018

Owner

The only thing I could find that needs attention is this typo here :-) It should probably be Some comand line arguments.

@danielquinn

This comment has been minimized.

Owner

danielquinn commented Feb 27, 2018

This is some really good code, and pretty code too! Thank you so much for this contribution! I have just that one typo nitpick to be fixed and then I can merge it.

As for tox, I think you're right on that. I was a bit overzealous in removing it when I was trying to push more into Travis. I'll put it back when I have some time unless you want to do that in this PR or a separate one.

@ovv

This comment has been minimized.

Contributor

ovv commented Feb 27, 2018

Thanks for the kind words. It should be good now.

@danielquinn

This comment has been minimized.

Owner

danielquinn commented Feb 28, 2018

Ok this looks good to me, but because this is such a dramatic change, I'm going to have to spend a little time trying it out before I merge it. Give me a couple days and I'll click the Big Green Button.

@danielquinn

This comment has been minimized.

Owner

danielquinn commented Mar 3, 2018

So I downloaded your code and ran it without issue, save for one problem: the --oneshot flag ran the consumer.run() alright, but the consumer loop has to run at least twice for anything to happen since the first run just takes file statistics to ensure we don't consume a file that's currently being written to.

I made that one modificaiton and then merged all of your code into master. Thank you so much for this excellent body of work! It's annoying how Github now says that this branch has conflicts, but I guess that's what happens when I tweak something before a merge. Oh well.

I'll be doing a release this evening with your modifications. Thanks again!

@danielquinn danielquinn closed this Mar 3, 2018

@danielquinn

This comment has been minimized.

Owner

danielquinn commented Mar 3, 2018

On second thought, I think I'd like to make sure #317 isn't a problem before I release. If you're just running a checkout of master though, a git pull will have you running the latest. Otherwise, I'm going to wait to see what happens with that thread before I release 1.4.0.

@ovv

This comment has been minimized.

Contributor

ovv commented Mar 4, 2018

Good catch ! Thanks for the fix & merge. A new release is not urgent for me, I'm running on master at the moment.

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