Opened 4 years ago

Closed 4 years ago

#27062 closed enhancement (fixed)

MR5: note on what happens if an executable script is loaded

Reported by: galois Owned by:
Priority: major Milestone: sage-8.7
Component: documentation Keywords:
Cc: embray Merged in:
Authors: Dima Pasechnik Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 74c1d22 (Commits, GitHub, GitLab) Commit: 74c1d22288a75eb242d621da92926a6be81acad0
Dependencies: Stopgaps:

Status badges

Description (last modified by dimpase)

Dmitrii Pasechnik (@dimpase) opened a merge request at https://gitlab.com/sagemath/sage/merge_requests/5:







Change History (36)

comment:1 Changed 4 years ago by dimpase

  • Authors changed from Dmitrii Pasechnik to Dima Pasechnik
  • Component changed from PLEASE CHANGE to documentation
  • Description modified (diff)
  • Milestone changed from sage-8.6 to sage-8.7
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 4 years ago by galois

  • Commit changed from 88c37efe4b36f406f5df001afc596dfffb4cd369 to 73825d453d18a7bd1811aea2a0522013e51c49dd

New commits added to merge request. I updated the commit SHA-1. New commits:

73825d4cleaned up spaces

comment:3 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_info

Erik seems to think that this is a bug.

comment:4 Changed 4 years ago by dimpase

  • Cc embray added

I don't think it is a bug. In Sage REPL you have __name__==__main__, and load() is akin to typing code in. Why would it go under another __name__?

comment:5 follow-up: Changed 4 years ago by embray

I think it's a principle of least astonishment. We teach that the way to make a Python script into an executable command-line script is to put if __name__ == '__main__':. Most users don't really know what that means or why it works; it's just cargo cult. While load() is not perfectly analogous to import and, as you say, is more like typing in commands at the REPL, I don't think it should run those commands with __name__ = '__main__' since that will break the user's expectation that "that code will only run when I run this file as a script".

comment:6 in reply to: ↑ 5 Changed 4 years ago by dimpase

Replying to embray:

I think it's a principle of least astonishment. We teach that the way to make a Python script into an executable command-line script is to put if __name__ == '__main__':. Most users don't really know what that means or why it works; it's just cargo cult. While load() is not perfectly analogous to import and, as you say, is more like typing in commands at the REPL, I don't think it should run those commands with __name__ = '__main__' since that will break the user's expectation that "that code will only run when I run this file as a script".

Sorry, but at Python REPL you do have __name__==__main__. So you propose to temporarily set __name__ to something else, then set it back, emulating import .... So then you'd also need to explain in the docs what __name__ it is, decide whether it will be a proper import, and so reflected in sys.modules.keys(), with a potential for name clashes, or not, and (very confusing, potentially) etc...

No, I really do not like this. I'd rather document the current behaviour and be done with it.

comment:7 Changed 4 years ago by dimpase

By the way, ipython has a similar facility, a magic %load:

In [4]: %load p.py

In [5]: # %load p.py
   ...: if __name__ == '__main__':
   ...:     print("blah")
   ...: 
blah

In [6]: 

So you propose to deviate from this, too....

comment:8 follow-up: Changed 4 years ago by embray

Yeah, I think that's bad, actually, in IPython too. There's no good reason to have __name__ == '__main__' in this context.

comment:9 in reply to: ↑ 8 Changed 4 years ago by dimpase

Replying to embray:

Yeah, I think that's bad, actually, in IPython too. There's no good reason to have __name__ == '__main__' in this context.

In ipython it's obvious that %load is nothing but pasting into REPL, no? And why would such a pasting do something funny with __name__?

comment:10 Changed 4 years ago by embray

I don't think Sage's load() function necessarily has to be exactly analogous to IPython's %load magic, but I am beginning to come around to your point.

I believe what's really missing here is a way to import .sage files as though they were normal Python modules.

I think this would be really easy to do too, so maybe I'll just do that (and accept this documentation note in the meantime).

comment:11 Changed 4 years ago by dimpase

  • Status changed from needs_info to needs_review

Well, perhaps something like a Python extension to Sage-preparse whatever comes in import statements, or indeed a module_name= parameter for load() and attach(), but this is certainly something tricky and of very low priority.

comment:12 Changed 4 years ago by embray

It's actually pretty easy (though ironically it was easier in the old import system).

comment:13 Changed 4 years ago by embray

Created a ticket for this at #27074. I'm surprised I haven't made one for this already but I don't think I have...

comment:14 Changed 4 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to needs_work

I made a merge request to your merge request with a suggested slight rewording, but if you accept that then positive review otherwise.

comment:15 follow-up: Changed 4 years ago by dimpase

I don't seem to get anything on gitlab that points to your request - could you tell me where to look?

there is nothing on https://gitlab.com/sagemath/sage/merge_requests/5 to do for me, in fact.

But I just noticed in a merge request in my fork, duh... All done now.

Last edited 4 years ago by dimpase (previous) (diff)

comment:16 Changed 4 years ago by galois

  • Commit changed from 73825d453d18a7bd1811aea2a0522013e51c49dd to d5ca638e7096cc7fd71c59fa78a42d64381adace

New commits added to merge request. I updated the commit SHA-1. New commits:

52bfd1fUpdate load.py
d5ca638Merge branch 'patch-1' into 'develop'

comment:17 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by jdemeyer

Replying to dimpase:

I don't seem to get anything on gitlab that points to your request - could you tell me where to look?

there is nothing on https://gitlab.com/sagemath/sage/merge_requests/5 to do for me, in fact.

This is exactly the reason why I say that the Sage Trac workflow is better than GitLab or GitHub.

comment:18 in reply to: ↑ 17 ; follow-ups: Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

I don't seem to get anything on gitlab that points to your request - could you tell me where to look?

there is nothing on https://gitlab.com/sagemath/sage/merge_requests/5 to do for me, in fact.

This is exactly the reason why I say that the Sage Trac workflow is better than GitLab or GitHub.

Well, I don't think it is bad - it's just different - you need to know where to look, and this is actually quite intuitive. And it is better, as there is no need to mess with branches manually. (unless you work in public/, with all the possible gotcha's).

comment:19 in reply to: ↑ 18 Changed 4 years ago by dimpase

Replying to dimpase:

Replying to jdemeyer:

Replying to dimpase:

I don't seem to get anything on gitlab that points to your request - could you tell me where to look?

there is nothing on https://gitlab.com/sagemath/sage/merge_requests/5 to do for me, in fact.

This is exactly the reason why I say that the Sage Trac workflow is better than GitLab or GitHub.

Well, I don't think it is bad - it's just different - you need to know where to look, and this is actually quite intuitive. And it is better, as there is no need to mess with branches manually. (unless you work in public/, with all the possible gotcha's).

(And you erased the line I added to my comment, which meant to indicate that even an old demented guy like me, who learned to program with punchcards can still figure it out himself :-P)

comment:20 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

Replying to dimpase:

you need to know where to look

Well, I shouldn't need to know where to look. With the Sage Trac workflow, everything is in one place.

comment:21 follow-up: Changed 4 years ago by dimpase

No, it's not in one place - it's on your computer, on the ticket's author's computer, in different git branches, on trac, on trac's git server, whereas on gitlab it's all there. Things can, and do, go wrong in many different ways.

Whereas on gitlab the review and tests (something not quite ready yet) are all done online. It's only cause I'm a very unexperienced user of gitlab, I did ask that question is the 1st place (and I didn't even check my repo for notification - I could have set up an email notifications and got it in mailbox then, totally clear).

comment:22 in reply to: ↑ 21 Changed 4 years ago by embray

Replying to dimpase:

Whereas on gitlab the review and tests (something not quite ready yet) are all done online. It's only cause I'm a very unexperienced user of gitlab, I did ask that question is the 1st place (and I didn't even check my repo for notification - I could have set up an email notifications and got it in mailbox then, totally clear).

Yes, normally you'd get an e-mail notification in which case it's clearer. Though when I made my MR I should have included a reference to yours, and it would have automatically created a backreference as well.

comment:23 Changed 4 years ago by embray

  • Status changed from needs_work to positive_review

comment:24 Changed 4 years ago by galois

  • Commit changed from d5ca638e7096cc7fd71c59fa78a42d64381adace to 5389d8dc87d79db7b81af492edf53a32699e8323

New commits added to merge request. I updated the commit SHA-1. This was a forced push. New commits:

comment:25 Changed 4 years ago by galois

  • Commit changed from 5389d8dc87d79db7b81af492edf53a32699e8323 to 1a954dcd4ddfec0cf4f19bfef5f70fbc8a1aac8b

New commits added to merge request. I updated the commit SHA-1. New commits:

1a954dcdelete outdated NOTE about nauty

comment:26 Changed 4 years ago by dimpase

  • Branch changed from u/galois/mrs/5/develop to u/dimpase/GQ
  • Commit changed from 1a954dcd4ddfec0cf4f19bfef5f70fbc8a1aac8b to 5389d8dc87d79db7b81af492edf53a32699e8323
  • Status changed from positive_review to needs_work

I messed up the gitlab fork by pushing with -f into my repo (yes, mine, not that galois guy!)

and the work on this ticket is just gone now :-(

This is obviously a workflow bug that must be somehow fixed...

comment:27 Changed 4 years ago by dimpase

  • Branch changed from u/dimpase/GQ to u/galois/mrs/5/develop
  • Commit changed from 5389d8dc87d79db7b81af492edf53a32699e8323 to 1a954dcd4ddfec0cf4f19bfef5f70fbc8a1aac8b

New commits:

1a954dcdelete outdated NOTE about nauty

comment:28 Changed 4 years ago by dimpase

  • Branch changed from u/galois/mrs/5/develop to u/dimpase/GQ
  • Commit changed from 1a954dcd4ddfec0cf4f19bfef5f70fbc8a1aac8b to a0f6f4e0ac78942de47c233fde8136be9240710f
  • Status changed from needs_work to positive_review

I have cherry-picked the patch from Erik's Sage gitlab fork - fortunately it was still there.

comment:29 follow-up: Changed 4 years ago by tscrim

It might be good in terms of formatting to do

-    code is inside an
+    code is inside an::
 
-            if __name__ == "__main__":
+        if __name__ == "__main__":
 

(The extra indentation I think is irrelevant.) But this is a trivial thing.

comment:30 Changed 4 years ago by git

  • Commit changed from a0f6f4e0ac78942de47c233fde8136be9240710f to 74c1d22288a75eb242d621da92926a6be81acad0
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

74c1d22trimming the space

comment:31 in reply to: ↑ 29 Changed 4 years ago by dimpase

  • Status changed from needs_review to positive_review

Replying to tscrim:

It might be good in terms of formatting to do

-    code is inside an
+    code is inside an::
 
-            if __name__ == "__main__":
+        if __name__ == "__main__":
 

(The extra indentation I think is irrelevant.) But this is a trivial thing.

fixed.

comment:32 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_info

Can you only set this to positive review once you actually are happy with it? This is now the second time that I'm backing out this ticket again after running it on the buildbot

comment:33 Changed 4 years ago by dimpase

  • Status changed from needs_info to positive_review

I am happy with it. I have no idea whether flipping its status has any bearing on anyone, sorry.

comment:34 Changed 4 years ago by embray

I'm not sure what you did because normally forced pushes work fine.

comment:35 Changed 4 years ago by dimpase

I made a gitlab branch, X, say, and a merge request from it. And then, I force-pushed to the branch X, overwriting whatever changes I made for the merge request, I think.

comment:36 Changed 4 years ago by vbraun

  • Branch changed from u/dimpase/GQ to 74c1d22288a75eb242d621da92926a6be81acad0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.