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

Allow sys.stdout to be replaced #88

Open
alanjds opened this issue Oct 6, 2018 · 13 comments
Open

Allow sys.stdout to be replaced #88

alanjds opened this issue Oct 6, 2018 · 13 comments
Labels
imported PR Pull Request imported from google/grumpy

Comments

@alanjds
Copy link

alanjds commented Oct 6, 2018

google#302 opened on Apr 28, 2017 by @alanjds

Please refer to the discussion on google#290

Do not merge until __getattr__ and __setattr__ is working.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by corona10
Friday Apr 28, 2017 at 15:51 GMT


I have one thing to discuss here.
At this point, I think that we should create module directory for these kinds of task. runtime/sysmodule.go => module/sysmodule.go
WDYT? @trotterdylan @alanjds

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by alanjds
Friday Apr 28, 2017 at 16:25 GMT


I totally agree, but am not mature enough on Golang to glance the implications of this change.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by corona10
Friday Apr 28, 2017 at 16:27 GMT


I believe that more modules will be needed. e.g (socketModule etc..)

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by alanjds
Saturday Apr 29, 2017 at 20:55 GMT


This is no more a WIP, after changed from __getattr__ to __getattribute__ fixed it.

Will continue the google#290 implementation over this code.

Please discuss what changes we want before this (#70) got be merged.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by trotterdylan
Saturday Apr 29, 2017 at 23:28 GMT


I would avoid __getattribute__ and instead use __getattr__. The latter is called only after nothing is found through normal attribute lookup, which is probably what we want in this case.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by alanjds
Sunday Apr 30, 2017 at 01:48 GMT


Me too. But this rabbit hole is getting too deep!
Let me close some Print() issues first and later I come back to implement __getattr__.

This __getattr__ issue seems at least one magnitude harder than the other issues anyway. I will need to get "slots" understanding first.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by alanjds
Monday May 01, 2017 at 19:00 GMT


Just to say, tracebacks would be very nice. Took me quite a bit to realize this 'res' typo

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by alanjds
Saturday Jun 10, 2017 at 15:48 GMT


I would avoid getattribute and instead use getattr. The latter is called only after nothing is found through normal attribute lookup, which is probably what we want in this case.

@trotterdylan Are you ok with going on this way, and allow to use getattribute for now, or is better to go develop getattr before keep going on this PR?

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by trotterdylan
Wednesday Jun 14, 2017 at 14:35 GMT


I think that __getattr__ should be fairly straightforward to implement. It needs a new slot and fallback logic in grumpy.GetAttr(). Also a few tests that validate the correct attribute resolution.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by alanjds
Friday Jun 16, 2017 at 16:40 GMT


Ok. Then I will chase __getattr__ on another PR.

Everything else is ok?

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by trotterdylan
Monday Jun 19, 2017 at 15:05 GMT


I think this is a little more complicated than I had hoped it would be. What if we get rid of the hybrid module concept and instead swap out sys.__dict__ something like this:

# lib/sys.py
from __go__.grumpy import SysDict, SysModules
SysModules['sys'].__dict__ = SysDict
... carry on with the module as normal

Currently __dict__ is not settable, but it will be once google#331 is merged.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by alanjds
Monday Jun 19, 2017 at 18:46 GMT


Hum... the whole point of this PR is allow sys.stdout = StringIO() to propagate to print.

If SysModules['sys'].__dict__ = SysDict fixes this need and is simpler, lets do it!

Seems to still need an [SysDict[k] = locals()[k] for k in __all__] at the last line, but for me it looks more ok than the HybridModule stuff.

@alanjds
Copy link
Author

alanjds commented Oct 6, 2018

Comment by trotterdylan
Monday Jun 19, 2017 at 19:58 GMT


Right, so the trick would be get rid of the Stdout, Stdin and Stderr grumpy package members and instead pull those from SysDict like SysDict.GetItemString(f, "stdin"). I have a change started to see how this would work in practice. I can either finish that up and create a PR, or if you like, you can go that route in this PR.

Seems to still need an [SysDict[k] = locals()[k] for k in all] at the last line, but for me it looks more ok than the HybridModule stuff.

I don't think this is necessary if we do the __dict__ assignment at the very top of the module.

@alanjds alanjds added the imported PR Pull Request imported from google/grumpy label Oct 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported PR Pull Request imported from google/grumpy
Projects
None yet
Development

No branches or pull requests

1 participant