Add os.supports_* placeholders #964

youknowone · 2019-05-11T08:57:15Z

No description provided.

palaviv

Nice. Added a few comments

palaviv · 2019-05-11T09:24:06Z

vm/src/stdlib/os.rs

@@ -512,33 +577,128 @@ pub fn make_module(vm: &VirtualMachine) -> PyObjectRef {
         "st_size" => ctx.new_property(StatResultRef::st_size),
    });

-    py_module!(vm, "_os", {
-        "open" => ctx.new_rustfunc(os_open),
+    let fd_funcs: Vec<(&str, PyObjectRef, Option<bool>, Option<bool>, Option<bool>)> = vec![


Why is the support_* an Option? This can be either true of false. You convert the None to false later

It is a kind of documentation describing it is varies by platform(Some) or always not supported(None)

I am not sure I understand...
Let's take os.symlink for example. on linux system dir_fd is supported and on windows it is not. In that case we will have Some(true) on linux and Some(false) on windows. While follow_symlinks will always be None. Is there any difference between Some(false) and None? Would there be a different error message somewhere I am missing?

There is no difference between None and Some(false) for implementations. As you pointed out, None marks os.symlink doesn't have any option about follow_symlinks. I think it is a little bit confusing because it is just Some(false) for now.
Once we get actual values for each platforms, the None part always will be None. Because Python spec doesn't have any value about it. Otherwise, the Some part will be something like Some(platform_supports_os_symlink) because Python spec describes the value must be true or false by platforms.

Is platform_supports_os_symlink a function in your example or a boolean value?

It is a value. (or result of a function) I think the values are platform-specific constants defined in compile time.

My opinion is that this Option is not needed. I think using platform_supports_os_symlink or false would be more readable.

palaviv · 2019-05-11T09:26:11Z

tests/snippets/stdlib_os.py

+# supports
+assert isinstance(os.supports_fd, set)
+assert isinstance(os.supports_dir_fd, set)
+assert isinstance(os.supports_follow_symlinks, set)


Can you test that os.stat (or any other compatible function) is in os.supports_*?

Nothing is insdie of the set for now. It is just placeholders. See fd_funcs which only includes None and Some(false)

Cool. Did not see that.

palaviv · 2019-05-11T09:30:17Z

vm/src/stdlib/os.rs

+        // chmod Some Some Some
+        // chown Some Some Some
+        // chroot Some None None
+        (


This is not very readable in the current way. It is hard to remember what parameter is which. How about changing this to be a vector of:

struct FdFunc { name: String, func_obj: PyObjectRef, supports_fd: boolean, supports_dir_fd: boolean, supports_follow_symlinks: boolean, }

I fixed this and other comments, thanks!

palaviv · 2019-05-11T09:33:29Z

vm/src/stdlib/os.rs

+            None,
+        ),
+        // utime Some Some Some
+        // walk Some None


Some of this function like walk are implemented in pure python. Maybe consider removing the placeholders?

palaviv · 2019-05-11T09:45:24Z

vm/src/stdlib/os.rs

    use std::os::unix::fs as unix_fs;
+    let src = make_path(vm, src, &dir_fd);


The dir_fd in symlink is only relevant for the dst. The underlying implementation use symlinkat.

codecov-io · 2019-05-11T10:26:15Z

Codecov Report

Merging #964 into master will decrease coverage by 0.03%.
The diff coverage is 62.85%.

@@            Coverage Diff             @@
##           master     #964      +/-   ##
==========================================
- Coverage   64.84%   64.81%   -0.04%     
==========================================
  Files          91       91              
  Lines       16312    16363      +51     
  Branches     3695     3702       +7     
==========================================
+ Hits        10578    10606      +28     
- Misses       3277     3293      +16     
- Partials     2457     2464       +7

Impacted Files	Coverage Δ
vm/src/obj/objset.rs	`73.45% <100%> (ø)`	⬆️
vm/src/stdlib/os.rs	`68.4% <62.31%> (-2.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95a894a...64afd5c. Read the comment docs.

palaviv

LGTM

palaviv reviewed May 11, 2019

View reviewed changes

youknowone force-pushed the os-supports-placeholders branch from bfcc82d to 958767d Compare May 12, 2019 10:10

Add os.supports_* placeholders

64afd5c

youknowone force-pushed the os-supports-placeholders branch from 958767d to 64afd5c Compare May 12, 2019 16:15

palaviv approved these changes May 12, 2019

View reviewed changes

coolreader18 merged commit b01d756 into RustPython:master May 12, 2019

youknowone deleted the os-supports-placeholders branch May 13, 2019 03:10

		use std::os::unix::fs as unix_fs;
		let src = make_path(vm, src, &dir_fd);

Add os.supports_* placeholders #964

Add os.supports_* placeholders #964

Uh oh!

Conversation

youknowone commented May 11, 2019

Uh oh!

palaviv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

palaviv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented May 11, 2019 •

edited

Loading