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

Cleanup code by removing if __name__ == '__main__' pattern #1922

Merged
merged 1 commit into from
Nov 22, 2016

Conversation

Tarrasch
Copy link
Contributor

This patch started out as fixing the red in the current travis flake8
step. As most occurrences was about simply removing the over-(mis)-used
if __name__ == "__main__" pattern that was way to prevalent in the luigi
code base.

This is kind of analogous to the other time flake8 suddenly got more
sensitive. See #1786

@mention-bot
Copy link

@Tarrasch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @freider, @hadesbox and @erikbern to be potential reviewers.

@dlstadther
Copy link
Collaborator

@Tarrasch , Are you saying that $ luigi --module examples.foo examples.Foo --workers 2 --local-scheduler is the only "proper" way to execute luigi code and that if __name__ == '__main__' shouldn't be used? Why is this?

@Tarrasch
Copy link
Contributor Author

I believe that to be the case yes. I think there's a few advantages:

  • Less boilerplate
  • Your file is even more "declarative". I think the luigi API should have a declarative feel, just like GNU make.
  • Reading a line like python src/python/blah/blah/luigi_tasks.py ... feels like it could do anything, having something like luigi --module blah/blah/luigi_tasks ... is clearer that it's about starting luigi.
  • Python community in general seem to be for "have only one way to do things". I don't like that we're currently not consistent in our docs and code.

Come to think about it. I made a similar statement when I deprecated luigi.run().

Does all this make sense to you? :)

@erikbern
Copy link
Contributor

I don't mind the `if name == 'main`` personally. I generally hate overly prescriptive frameworks that impose rules on how to execute stuff. But I guess this is only internal cleanup so LGTM

@dlstadther
Copy link
Collaborator

Shoot, I'm just behind on the times then. Perhaps due to my comfort with Python, I've always used

if __name__ == "__main__":
    luigi.run(['NameOfMyTopLevelTask', '--some-option', 'the_value'])

Since I don't print or see my non-errors, I was unaware of luigi.run deprecation.

I'll switch over to luigi --module /path/to/file.py TaskName --option value

This patch started out as fixing the red in the current travis flake8
step.  As most occurrences was about simply removing the over-(mis)-used
`if __name__ == "__main__"` pattern that was way to prevalent in the luigi
code base.

This is kind of analogous to the other time flake8 suddenly got more
sensitive. See spotify#1786
@Tarrasch
Copy link
Contributor Author

Reviewers: Seems like the actual flake8 issues were fixed in a540006 already, thanks @mrunesson. :)


@dlstadther, I'm still not sure if luigi.run was a correct thing. Please feedback how using luigi plays out for you.

@Tarrasch Tarrasch changed the title Cleanup code and adhere to E305 Cleanup code by removing if __name__ == '__main__' pattern Nov 21, 2016
@Tarrasch Tarrasch merged commit aace9d3 into spotify:master Nov 22, 2016
@Tarrasch Tarrasch deleted the fix_flake8 branch November 22, 2016 08:20
@erikbern
Copy link
Contributor

@dlstadther in that case there's no need to involve command line parsing. iirc you can just do luigi.build([NameOfMyTask('banana')])

Tarrasch added a commit to Tarrasch/luigi that referenced this pull request Dec 28, 2016
This is exactly like spotify#1922, only
that I forgot to search for '__main__' with single qoutes. This patch
completes that PR.
Tarrasch added a commit that referenced this pull request Dec 28, 2016
This is exactly like #1922, only
that I forgot to search for '__main__' with single qoutes. This patch
completes that PR.
kreczko pushed a commit to kreczko/luigi that referenced this pull request Mar 28, 2017
This is exactly like spotify#1922, only
that I forgot to search for '__main__' with single qoutes. This patch
completes that PR.
This was referenced Jun 29, 2022
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.

4 participants