Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Allow sys.stdout to be replaced #302

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

alanjds
Copy link
Contributor

@alanjds alanjds commented Apr 28, 2017

Please refer to the discussion on #290

Do not merge until __getattr__ and __setattr__ is working.

(should I follow the Google 2-space or PEP 4-space?)
Needs working __getattr__/__setattr__.

Please wait if before merge
@corona10
Copy link
Contributor

corona10 commented Apr 28, 2017

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
Contributor Author

alanjds commented Apr 28, 2017

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

@corona10
Copy link
Contributor

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

@alanjds alanjds changed the title [WIP] Needed to fix #290: Allow sys.stdout to be replaceble Allow sys.stdout to be replaced Apr 29, 2017
@alanjds
Copy link
Contributor Author

alanjds commented Apr 29, 2017

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

Will continue the #290 implementation over this code.

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

@trotterdylan
Copy link
Contributor

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
Contributor Author

alanjds commented Apr 30, 2017

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
Contributor Author

alanjds commented May 1, 2017

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

@alanjds
Copy link
Contributor Author

alanjds commented Jun 10, 2017

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?

@trotterdylan
Copy link
Contributor

trotterdylan commented Jun 14, 2017

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
Contributor Author

alanjds commented Jun 16, 2017

Ok. Then I will chase __getattr__ on another PR.

Everything else is ok?

@trotterdylan
Copy link
Contributor

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 #331 is merged.

@alanjds
Copy link
Contributor Author

alanjds commented Jun 19, 2017

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.

@trotterdylan
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants