Skip to content

Add "u" prefix in repr() of usys objects. #7709

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

Closed
wants to merge 2 commits into from

Conversation

laurensvalk
Copy link
Contributor

@laurensvalk laurensvalk commented Aug 26, 2021

This follows up on @stinos' #6164.

This updates

>>> repr(usys.stdout)
'<io.TextIOWrapper 1>'

to

>>> repr(usys.stdout)
'<uio.TextIOWrapper 1>'

in order to match the module it is imported from.

I've also updated a few comments in mpconfig to add the prefix.

EDIT: Fix tests/extmod/vfs_fat_fileio1.py not passing.

This commit changes

>>> repr(usys.stdout)
'<io.TextIOWrapper 1>'

to

>>> repr(usys.stdout)
'<uio.TextIOWrapper 1>'

in order to match the module it is imported from.
This was done for some comments already, but not all of them.
@stinos
Copy link
Contributor

stinos commented Aug 26, 2021

Can you explain the reason for this change? I.e. why is it preferred to 'break' things the other way? I mean it used to be

>>> repr(sys.stdout)
'<io.TextIOWrapper 1>'

which will now be

>>> repr(sys.stdout)
'<uio.TextIOWrapper 1>'

so that's still a mismatch, just different.

@jimmo
Copy link
Member

jimmo commented Aug 26, 2021

Thanks @laurensvalk

In #5377 (which was recently merged and undoes the documentation part of #6164), I made the case that for most users the module are called "foo" not "ufoo". The "u" prefix is an implementation detail that allows for the module to be extended by Python (and allowing this was the part purpose of @stinos' #6164).

With the exception of some leftovers in extmod, all the modules are defined in modfoo.c, the "u" prefix is only applied at module registration. Some of the tests need to use import ufoo because there exists a "tests/foo" directory and import ufoo is a convenient way to work around this (this is true for tests/sys for example).

I think the guiding principle should be that ideally most users of MicroPython are never aware of the "u" prefix. They should always write "import foo" in their Python code (with the exception of uctypes (see #5377 (comment)), and for now at least, uasyncio). So in your example, most users should have written

>>> import sys
>>> repr(sys.stdout)
'<io.TextIOWrapper 1>'

So I think I would argue that the current handling of repr is correct, but I agree this is an interesting case! Do you have more background from pybricks?

@laurensvalk
Copy link
Contributor Author

Can you explain the reason for this change?

This is the current behavior:

>>> import io
>>> io
<module 'uio'>

It seemed reasonable to do the same for its contents, so uio.TextIOWrapper.

@laurensvalk
Copy link
Contributor Author

laurensvalk commented Aug 26, 2021

And regarding the change to py/mpconfig, there do seem to be some inconsistencies. For example, the following snippet has comments referring to both ucollections and collections. Picking one version could make sense.

micropython/py/mpconfig.h

Lines 1142 to 1150 in 145fede

// Whether to provide "collections" module
#ifndef MICROPY_PY_COLLECTIONS
#define MICROPY_PY_COLLECTIONS (1)
#endif
// Whether to provide "ucollections.deque" type
#ifndef MICROPY_PY_COLLECTIONS_DEQUE
#define MICROPY_PY_COLLECTIONS_DEQUE (0)
#endif

@jimmo
Copy link
Member

jimmo commented Aug 26, 2021

This is the current behavior (repr(io) --> uio)

This is confusing... and inconsistent, e.g. sys is sys, but most have the u prefix.

I think I'd argue that they should all be "foo" not "ufoo". I don't think fixing this would be a backwards compatibility issue...? You can still tell the difference between foo.py vs builtin-foo with the existence of foo.path.

Note: everything is registered as ufoo (and it should stay this way in order to make weak links work), it's just name that should change, for the following modules:

uarray
ubinascii
ubluetooth
ucollections
ucryptolib
uerrno
uhashlib
uio
ujson
umachine
uos
urandom
ure
uselect
usocket
ussl
ustruct
utime
uzlib

That's 20 bytes! (maybe)

Will be interested to see what @dpgeorge thinks!

For example, the following snippet has comments referring to both ucollections and collections. Picking one version could make sense.

Yeah I think this second case "ucollections.deque" should be "collections.deque".

@stinos
Copy link
Contributor

stinos commented Aug 26, 2021

I don't think fixing this would be a backwards compatibility issue...?

I think repr is supposed to by symmetrical with eval so that's where to look for problems?

In general I agree that foo should be used instead of ufoo everywhere except in foo itself (if it exists), so seems to make sense to drop the u everywhere as well, except where needed.

@laurensvalk
Copy link
Contributor Author

I originally opened this PR to address the inconsistency, not to argue one way or the other, but since you ask, here it goes:

I see modules like ustruct and usys as the real MicroPython modules, which can be entirely self-consistent. In this view, struct and sys are just import aliases at the highest level.

I made the case that for most users the module are called "foo" not "ufoo". (...) I think the guiding principle should be that ideally most users of MicroPython are never aware of the "u" prefix. They should always write "import foo" in their Python code

Do you have more background from pybricks?

I would personally argue the opposite. In our experience with beginning users, it's a lot easier to say, "MicroPython works exactly like this", as opposed to "it's like Python, except for these 100+ differences".

This approach does require such exact documentation for MicroPython modules, which is what we're currently working on. It's currently somewhat specific to our target boards (LEGO hubs), but in principle it can be adapted for MicroPython more generally if there's any interest.

@stinos
Copy link
Contributor

stinos commented Aug 27, 2021

In our experience with beginning users, it's a lot easier to say, "MicroPython works exactly like this", as opposed to "it's like Python, except for these 100+ differences".

I might be looking at it from a different angle, but from my point of view using not u looks like the better situation in this regard:

  • one can just tell beginners that if they need an array, they import array and if they need os features, they import os. That is simpler and easier to understand than adding an extra character there. And one doesn't have to explain why that character is there.
  • MicroPython is Python with 100+ differences. At one point or another that has to be explained to users. Not using u though means actually one less difference because common modules have the same name.

@davehylands
Copy link
Contributor

The intent of having the "u" prefix was that the only place that should need to use this prefix is a module which is building ontop of one of the u modules to add missing functionality.

i.e. you're creating an os module which extends the functionality of the uos module. In this case, the os.py needs to import uos. In all other cases, the intent was that the user would just use import os.

@laurensvalk
Copy link
Contributor Author

For what it's worth as far as the original PR is concerned, CPython has this behavior:

>>> import sys
>>> sys.stdout
<_io.TextIOWrapper name='<stdout>' mode='w' encoding='UTF-8'>

Other than using _ in place of u, it's quite like what's proposed here.

@laurensvalk
Copy link
Contributor Author

laurensvalk commented Aug 27, 2021

I'll also add that this particular PR isn't really all that important.

The module name discussion itself is more relevant, but it seems that this is being discussed in various other places already.

I'm happy to just update this PR when that gets decided. If such a naming convention exists already, just let me know.

Edit: I see now that #5377 removes u from the documentation everywhere. Thanks for the link @jimmo. If that means u should also be removed in cases like this PR, I'm happy to update the PR accordingly.

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Oct 15, 2021
@laurensvalk
Copy link
Contributor Author

This is superseded by #9018, so we can probably close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants