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

MRG: Add __file__ #166

Merged
merged 1 commit into from Nov 16, 2016

Conversation

Projects
None yet
3 participants
@larsoner
Contributor

larsoner commented Nov 16, 2016

In one of my scripts I want to use __name__ but in master it is not available.

@larsoner larsoner referenced this pull request Nov 16, 2016

Merged

MRG: TDT delay #283

@larsoner

This comment has been minimized.

Contributor

larsoner commented Nov 16, 2016

... and by __name__ I mean __file__

@larsoner larsoner changed the title from WIP: Add __name__ to MRG: Add __file__ Nov 16, 2016

@larsoner

This comment has been minimized.

Contributor

larsoner commented Nov 16, 2016

Travis is happy, ready for review/merge from my end

@lesteve

This comment has been minimized.

Contributor

lesteve commented Nov 16, 2016

LGTM, merging thanks!

@lesteve lesteve merged commit 5771501 into sphinx-gallery:master Nov 16, 2016

1 check passed

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

This comment has been minimized.

Contributor

lesteve commented Nov 16, 2016

@Eric89GXL actually thinking about it more, example_file is something like auto_examples/plot_seaborn.py, is that what you expected? I would have thought that it should point to the example i.e. either examples/plot_seaborn.py or maybe even better its absolute path.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Nov 16, 2016

@lesteve

This comment has been minimized.

Contributor

lesteve commented Nov 16, 2016

Yeah pointing to the actual original python file would be best

PR welcome ;-) !

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Nov 16, 2016

I just thought about something: it seems that name is defined in an IPython notebook, but not file.

I would hence be in favor of not supporting file. It's going to confuse users that run things in IPython notebooks.

@larsoner larsoner deleted the larsoner:file branch Nov 16, 2016

@larsoner

This comment has been minimized.

Contributor

larsoner commented Nov 16, 2016

That's true. However, any example that a given project has that uses __file__ already has this problem, and thus we aren't making anything worse by supporting it, except maybe because of the IPython-notebook download button. Maybe we should disable that for any example file that uses __file__?

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Nov 16, 2016

That's true. However, any example that a given project has that uses file
already has this problem, and thus we aren't making anything worse by
supporting it, except maybe because of the IPython-notebook download button.

The problem is that users are strongly demanding the IPython notebook
feature. These users expect examples to work in IPython notebooks.

Maybe we should disable that for any example file that uses file?

Why do examples need the file? IMHO they should be rewritten. People will
copy-paste them in IPython notebooks and run into problems.

By supporting file in sphinx-gallery you are just hidding this
problem.

The core problem is that Python now has several execution models and that
they are not compatible. It's a problem created by notebook, and not by
sphinx-gallery.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Nov 16, 2016

Fair enough. Feel free to roll back the commit in master

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Nov 16, 2016

@larsoner

This comment has been minimized.

Contributor

larsoner commented Nov 16, 2016

Yeah, after some tweaking I got it working using a different tactic than __file__. I don't know if it will work in all cases, but at least it did here.

GaelVaroquaux added a commit that referenced this pull request Nov 16, 2016

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Nov 16, 2016

I reverted, but stopped short of documenting this behavior, because if
it's a major blocker for many people we will support file.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Nov 16, 2016

Good catch @GaelVaroquaux !

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