Introduce `__text_signature__` property by moreal · Pull Request #2904 · RustPython/RustPython

moreal · 2021-08-18T00:55:38Z

This pull request tries to resolve #2410

This pull request does:

Fill 'object' type's missing docstrings 0b52d0c
Introduce new __text_signature__ property to PyType, PyBuiltinMethod, PyBuiltinFunction. 40ab1c4

Code to verify compatibility

print(object.__str__.__text_signature__)  # expected `($self, /)`
print(object.__text_signature__)  # expected `()`
print(type.__text_signature__)  # expected `None` (no output)

References

parse __text_signature__ logic: https://github.com/python/cpython/blob/fa919fdf2583bdfead1df00e842f24f30b2a34bf/Objects/typeobject.c#L183-L203

moreal · 2021-08-18T01:04:14Z

This pull request seems to resolve #2680 in some part 👀

youknowone · 2021-08-18T01:50:28Z

        format!("<built-in function {}>", self.value.name)
    }
+    #[pyproperty(magic)]
+    fn text_signature(&self, vm: &VirtualMachine) -> PyObjectRef {


Suggested change

fn text_signature(&self, vm: &VirtualMachine) -> PyObjectRef {

fn text_signature(&self, vm: &VirtualMachine) -> Option<String> {

With this signature, None will turn into Python None and Some(String) will turn into Python str

I tried to apply it in d12d286 commit. Could you check it again?

DimitrisJim · 2021-08-18T01:56:16Z

This is nice! It does introduce an issue though, __doc__ now contains that text signature in its contents, you'll need to tweak doc to ignore the signature at the start (similar to CPythons _PyType_DocWithoutSignature).

moreal · 2021-08-18T12:43:06Z

This is nice! It does introduce an issue though, __doc__ now contains that text signature in its contents, you'll need to tweak doc to ignore the signature at the start (similar to CPythons _PyType_DocWithoutSignature).

Yeah, I missed it to comment. 😅 I made it as an issue #2907.

youknowone

nice work, I left a few comments about code style.

youknowone · 2021-08-18T14:58:31Z

+fn find_signature(name: &str, doc: &str) -> Option<String> {
+    let dot_index = name.rfind('.');
+    let name = match dot_index {
+        Some(index) => name[index + 1..].to_owned(),
+        _ => name.to_owned(),
+    };
+
+    if !doc.starts_with(&name) {
+        return None;
+    }
+
+    let doc = doc[name.len()..].to_owned();
+    if !doc.starts_with('(') {
+        None
+    } else {
+        Some(doc)
+    }
+}


I feel like find_signature should return the first index of string of signature, but this function returns docs starting with signature paranthesis.

My suggestions:

if this function is just once used by get_text_signature_from_internal_doc, it doesn't look like to be an independent function.

If this function is planned to be used from somewhere, giving it a better name or comments will be a good idea. For now, I don't have a good name suggestion.

by the way, here is my suggestion for the code itself.

Suggested change

fn find_signature(name: &str, doc: &str) -> Option<String> {

let dot_index = name.rfind('.');

let name = match dot_index {

Some(index) => name[index + 1..].to_owned(),

_ => name.to_owned(),

};

if !doc.starts_with(&name) {

return None;

}

let doc = doc[name.len()..].to_owned();

if !doc.starts_with('(') {

None

} else {

Some(doc)

}

}

fn find_signature<'a>(name: &str, doc: &'a str) -> Option<&'a str> {

let name = name.rsplit('.').next().unwrap(); // rsplit always have first item

let doc = doc.strip_prefix(name)?;

if !doc.starts_with('(') {

None

} else {

Some(doc)

}

}

I feel like find_signature should return the first index of string of signature, but this function returns docs starting with signature paranthesis.

My suggestions:

1. if this function is just once used by `get_text_signature_from_internal_doc`, it doesn't look like to be an independent function. 2. If this function is planned to be used from somewhere, giving it a better name or comments will be a good idea. For now, I don't have a good name suggestion.

by the way, here is my suggestion for the code itself.

Thanks for suggestions. I looked CPython implementation again and there is the usage of find_signature and skip_signature at _PyType_DocWithoutSignature (see also #2907). Current skip_signature implementation is incorrect and cannot be reused in _PyType_DocWithoutSignature. So I will implement the functions to return index not str like you said your feel 🙏🏻 .

static const char * _PyType_DocWithoutSignature(const char *name, const char *internal_doc) { const char *doc = find_signature(name, internal_doc); if (doc) { doc = skip_signature(doc); if (doc) return doc; } return internal_doc; }

Oh, I didn't know the name was CPython referenced, I am sorry to confuse you. The CPython code looks like intending Option<&str>. So keep going with it.

I got the feeling from the convention of Rust library, but CPython has its own convention. So never mind about it.

As a note, I fixed skip_signature implementation but it is not used in this pull request so I removed it. And I implemented get_signature renamed from skip_signature.

youknowone · 2021-08-18T14:58:53Z

+fn skip_signature(doc: String) -> Option<String> {
+    doc.find(SIGNATURE_END_MARKER)
+        .map(|index| doc[..index + 1].to_owned())
+}


Suggested change

fn skip_signature(doc: String) -> Option<String> {

doc.find(SIGNATURE_END_MARKER)

.map(|index| doc[..index + 1].to_owned())

}

fn skip_signature(doc: &str) -> Option<&str> {

doc.find(SIGNATURE_END_MARKER)

.map(|index| &doc[..index + 1])

}

this functions doesn't need to copy doc

youknowone · 2021-08-18T15:16:11Z

+        let doc_string: Option<PyStrRef> = doc_string.and_then(|o| o.downcast().ok());
+        doc_string.and_then(|doc| {
+            get_text_signature_from_internal_doc(self.name().as_str(), doc.as_str())


Suggested change

let doc_string: Option<PyStrRef> = doc_string.and_then(|o| o.downcast().ok());

doc_string.and_then(|doc| {

get_text_signature_from_internal_doc(self.name().as_str(), doc.as_str())

let doc: PyStrRef = doc_string.and_then(|o| o.downcast().ok())?;

get_text_signature_from_internal_doc(self.name().as_str(), doc.as_str())

youknowone · 2021-08-18T15:24:18Z

+
+    #[pyproperty(magic)]
+    fn text_signature(&self) -> Option<String> {
+        let doc_string = self.get_attr("__doc__");


If you are interested in, I think tp_doc which should be added in PyTypeSlots is related to improvement of this line. It can be a topic for next patch.

I tried to apply your suggestion in 78ff936. Could you check that it was you said?

Co-Authored-By: Jeong YunWon <youknowone@users.noreply.github.com>

youknowone · 2021-08-20T17:04:06Z

it looks great, thank you for contributing!

moreal changed the title ~~Introduce '__text_signature__' property~~ Introduce __text_signature__ property Aug 18, 2021

youknowone requested changes Aug 18, 2021

View reviewed changes

moreal force-pushed the feature/add-text-signature-property branch from 2f06d9b to d12d286 Compare August 18, 2021 12:17

moreal mentioned this pull request Aug 18, 2021

Implement __doc__ to except signature #2907

Closed

moreal requested a review from youknowone August 18, 2021 12:42

youknowone requested changes Aug 18, 2021

View reviewed changes

youknowone reviewed Aug 18, 2021

View reviewed changes

moreal and others added 9 commits August 20, 2021 23:13

Fill 'object' type's docstring

a5f0594

Introduce '__text_signature__' property

8bc8c5b

Apply cargo fmt check

86a7c15

Apply clippy check

aeb4acb

Remove unnecessary python object creation

de91730

Apply clippy

13b2a0d

Refactor handling string

ad027a0

Co-Authored-By: Jeong YunWon <youknowone@users.noreply.github.com>

Correct skip_signature implementation

00b6d85

Remove unused skip_signature

0b6de47

moreal force-pushed the feature/add-text-signature-property branch from 34d5215 to 1931b9f Compare August 20, 2021 16:01

Load docstring from slots

78ff936

moreal force-pushed the feature/add-text-signature-property branch from 1931b9f to 78ff936 Compare August 20, 2021 16:02

moreal requested a review from youknowone August 20, 2021 16:04

youknowone approved these changes Aug 20, 2021

View reviewed changes

youknowone merged commit 216b597 into RustPython:master Aug 20, 2021

moreal mentioned this pull request Aug 20, 2021

Missing dataclasses module #2623

Closed

youknowone mentioned this pull request Aug 22, 2021

derive __text_signature__ from method definition #2923

Merged

youknowone added the z-ca-2021 Tag to track contrubution-academy 2021 label Oct 16, 2021

moreal mentioned this pull request Nov 20, 2021

Remove staled comments #3452

Merged

	fn text_signature(&self, vm: &VirtualMachine) -> PyObjectRef {
	fn text_signature(&self, vm: &VirtualMachine) -> Option<String> {

Uh oh!

Conversation

moreal commented Aug 18, 2021

Code to verify compatibility

References

Uh oh!

moreal commented Aug 18, 2021

Uh oh!

youknowone Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

moreal Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

DimitrisJim commented Aug 18, 2021

Uh oh!

moreal commented Aug 18, 2021

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

moreal Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

moreal Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moreal Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

youknowone commented Aug 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moreal Aug 19, 2021 •

edited

Loading

moreal Aug 20, 2021 •

edited

Loading

youknowone Aug 18, 2021 •

edited

Loading