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

Restructure TrollMoves scripts #20

Merged
merged 42 commits into from May 21, 2019

Conversation

@pnuu
Copy link
Contributor

commented Dec 11, 2018

This PR restructures all the "main" scripts of trollmoves. Most of client/server/mirror were duplicate code, and here most of that is now moved to trollmoves.move_it_base.py. At the same time some Python 3 compatibility updates and example configuration files were added.

@pnuu pnuu requested review from mraspaud and loerum Dec 11, 2018
Copy link
Member

left a comment

LGTM, haven't tested it though.

I have a couple of comments inline, and would also like to merge #13, #15, #16 and #17 before restructuring more if possible.

bin/move_it.py Outdated Show resolved Hide resolved
bin/move_it.py Show resolved Hide resolved
trollmoves/client.py Outdated Show resolved Hide resolved
@pnuu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

I haven't tested either, so don't know how much things are broken. Also agree on merging the other PRs first, and rebasing this after that is done.

@pnuu pnuu added this to In progress in Pytroll Contributor's Week - Spring 2019 - Madison, WI via automation May 9, 2019
Pytroll Contributor's Week - Spring 2019 - Madison, WI automation moved this from In progress to Needs review May 21, 2019
Copy link
Member

left a comment

Looks good, thanks for all the style cleaning ! There is one problem that prevents me from merging.

@@ -765,7 +770,7 @@ class CTimer(Thread):
t.cancel() # stop the timer's action if it's still waiting
"""

def __init__(self, interval, function, args=[], kwargs={}):
def __init__(self, interval, function, *args, **kwargs):

This comment has been minimized.

Copy link
@mraspaud

mraspaud May 21, 2019

Member

This change is not correct, args has to be passed as a list, kwargs as a dict as per the Thread call signature

This comment has been minimized.

Copy link
@pnuu

pnuu May 21, 2019

Author Contributor

Oh, right. Thought those original versions were considered "dangerous" and didn't think of the use-case at all. Good catch!

@mraspaud

This comment has been minimized.

Copy link
Member

commented May 21, 2019

As a note for future PRs, separating style cleaning/refactoring/bug fixes into different PRs would make it faster and easier to review :)

pnuu and others added 2 commits May 21, 2019
Pytroll Contributor's Week - Spring 2019 - Madison, WI automation moved this from Needs review to Reviewer approved May 21, 2019
@mraspaud mraspaud merged commit cb2f9dd into pytroll:master May 21, 2019
Pytroll Contributor's Week - Spring 2019 - Madison, WI automation moved this from Reviewer approved to Done May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.