Add listdir to unix/modos.c #3200
Conversation
There's no "os" module in micropython, only "uos". For uos, ilistdir() is "more standard".
Well, feel free to remove it from there ;-).
Unix port has os.listdir() (via micropython-lib). This patch conflicts with #2908, #3067 and https://github.com/micropython/micropython/wiki/ContributorGuidelines |
|
listdir() is a standard part of uos (not just os), documented in the docs, available in esp8266, stmhal and cc3200. It should be available in the unix port (under uos, not os) to make all the ports consistent. This PR here is also consistent with the VFS posix PR, since the latter uses the generic VFS functions to populate the uos module, which include listdir. |
| o->base.type = &mp_type_polymorph_iter; | ||
| o->dir = opendir(path); | ||
| o->iternext = listdir_next; | ||
| return MP_OBJ_FROM_PTR(o); |
There was a problem hiding this comment.
listdir() should return a list, not an iterator. See extmod/vfs.c for an implementation of listdir that uses ilistdir.
|
I'm afraid I'm not too familiar with micropython yet (as might be painfully obvious ;) ), but I'd be happy to make any necessary adjustments to the PR over the weekend. Thanks for your review! |
There was a problem hiding this comment.
listdir() is a standard part of uos (not just os),
It's not, or it would have been implemented before. It being not implemented is not omission, but a deliberate choice.
documented in the docs,
What docs document is (http://docs.micropython.org/en/latest/pyboard/library/index.html): "Any particular MicroPython variant or port may miss any feature/function described in this general documentation"
available in esp8266, stmhal and cc3200
These ports (in their default config) provide listdir() by the same reason as they "symlink" uos to os: to make it easier to use for novice users to use, and to provide os.listdir() without extra Python module. That doesn't make uos.listdir() standard, and its lack in Unix port exactly emphasizes this.
My assumption is that VFS subsystem provides standardized implementation of listdir in terms of ilistdir, without the need to patch it in each individual port, like this patch tries to do. But even after #3067 is merged, I think that for unix, uos.listdir() should not be enabled, to support the fact that portable unbloated MicroPython applications should use uos.ilistdir() (while CPython-compatible apps should use os.listdir()).
Applying that statement too often will lead to a mess of inconsistency and bad user experience (and leads to PRs like this one). There should be basic methods that are provided by all ports, and listdir() is a pretty basic method, learned by habit from using CPython, and which can be expected to exist for uPy.
listdir() has been there from the beginning and the addition of ilistdir() shouldn't mean the removal of listdir(). listdir() should remain standard, as part of uos. Otherwise uPy strays too much from CPy.
Making ilistdir() the standard means that uPy scripts have yet another thing which makes them incompatible with CPython. |
We have design principles, with motivation and usecases described in https://github.com/micropython/micropython/wiki/ContributorGuidelines . While patches are definitely welcome, random patches which go against the design principles above should not be a reason to against them. Btw, I don't remember your clear statement about the doc above, which based on multi-year cooperation history I treated as "agreement without words". Maybe it's good time to say something about it. And if "inconsistency" is the concern, an alternative solution was proposed - drop uos.listdir() from all ports. That should be as "good" as forcing superfluous function into each port.
A simple usecase: 1000 files, which isn't much. 8.3 naming, which is underestimation. 4 * 1000 + (8 + 1 + 3 + 1) * 1000 = 17K needed to be allocated by os.listdir() on ESP8266 port, which will likely fail for any non-trivial app. And it will never work for 10000 files, which is still not much. We don't need functions in API which don't work. And we can't afford to have 2 functions doing the same, especially if listdir is one-liner om top of listdir: But we go further and provided this convenience wrapper on some ports, mostly as means of "shell user interface" by listing filesystem directories, were there're no other means to do that. But on ports which don't have problem with listing files by other means, like Unix, there's no such builtin conveniences, and lack of it serves as a good hint that listdir() is not primary MicroPython API. And all the above talks about
There has never been
Yes, and we're already different enough. Let's enjoy our traits (being small) instead of endless useless attempts to become CPython. This is well explained in https://github.com/micropython/micropython/wiki/ContributorGuidelines.
There're 2 orthogonal ways to develop a MicroPython application:
These ways are well developed and well designed thru our history, e.g. way 2 is implemented in a way to not burden low-resource ports, for which way 2 is out of reach anyway. There's no need to marry those 2 ways, a lot of effort will be wasted, and only a monster will ensue. |
Yes there is a need. The entire point of MicroPython is that it's Python. |
|
Yes, it's a Python, and comments above discuss in details how it works in the case of os.listdir(), and https://github.com/micropython/micropython/wiki/ContributorGuidelines discusses in detail how it works in general. All that is status quo for few years. |
As I already said, listdir() has been the status quo since the beginning. |
|
You don't seem to respond to my arguments (or explicit requests for comments), just repeat some statements. So, you already said that listdir() was there from the beginning, I already said that there never has been builtin listdir() in unix port, where's the catch and who's wrong here then? |
My stance is that the bare-metal ports are the main targets, and unix is there a large part for testing the code and running scripts on the PC before on a bare-metal target. So unix should mirror bare-metal as much as possible. So it should add listdir(). |
|
And my stance is: We have unix coverage build for testing code, that can (and does) enable everything. Per the above arguments, listdir is less efficient that ilistdir, can be implemented with a one-liner in terms of it, and readily available via micropython-lib (so, if there're any issues, micropython/micropython-lib#47 would rather get more (well, any) attention). And enabling listdir additionally aggravates the issue: as well known, people don't read docs, so one would never know about ilistdir and will write apps which will fail on other ports (due to lack of memory, as explained above). Whereas now, someone trying to develop app/library "for MicroPython" (vs "for board XXX") would need to test it on unix port, and immediately spot the issue and will be motivated to think/learn new things. |
|
This would be fine to add but needs to be done differently, either in C as I suggest above (see VFS's listdir at https://github.com/micropython/micropython/blob/master/extmod/vfs.c#L433) or eventually in Python via #5025 |
os.listdiris more standard thanos.ilistdir(https://docs.python.org/3/library/os.html, https://docs.micropython.org/en/latest/pyboard/library/uos.html), and for example implemented on the esp32, so it'd be nice to have it in the unix tree as well.