Allow parsing source code directly#54
Conversation
|
Thanks for the PR! Me or Jukka will try to take a look next week. In the meantime, could you please resolve the merge conflict? |
| match pyo3::exceptions::PyUnicodeDecodeError::new_utf8( | ||
| obj.py(), | ||
| e.as_bytes(), | ||
| utf8_err, | ||
| ) { | ||
| Ok(err) => Self::Error::from_value(err.into_any()), | ||
| Err(err) => err, | ||
| } |
There was a problem hiding this comment.
Implementation taken from https://pyo3.rs/main/doc/src/pyo3/exceptions.rs#802-811 (PR https://github.com/PyO3/pyo3/pull/5668/changes), because new_err_from_utf8 is only in pyo3 0.29 which is not released yet.
The test is here: python/mypy@ac275e4
| )> { | ||
| serialize_module( | ||
| source, | ||
| PySourceType::Python, |
There was a problem hiding this comment.
I've hard-coded Python source type here, because mypy parsing functions never previously exposed an option to let the user treat source code directly as a .pyi stub source.
There was a problem hiding this comment.
I think it is better to use the same logic as for serialize_module().
|
Hmm...there might be some unnecessary allocations that are preventable. I'm going to do some benchmarking with another implementation. |
|
If we're willing to use Performance of |
hauntsaninja
left a comment
There was a problem hiding this comment.
This looks good to me, but will wait for Ivan or Jukka.
Mypy has dropped support for Python 3.9, so I would also take a PR dropping 3.9 support and increasing the stable ABI
ilevkivskyi
left a comment
There was a problem hiding this comment.
Generally looks good, but I have couple comments about respecting the file name when parsing source. It is better be consistent in case mypy (or other user) will decide to open/read file manually for some reason and pass the source. They should get identical result as when passing the file name.
| PySourceType::Python, | ||
| skip_function_bodies, | ||
| options, | ||
| false, |
There was a problem hiding this comment.
This should not be false, we should always respect the file name provided.
| )> { | ||
| serialize_module( | ||
| source, | ||
| PySourceType::Python, |
There was a problem hiding this comment.
I think it is better to use the same logic as for serialize_module().
|
@ilevkivskyi Whoops, I thought providing source contents was only used via |
ilevkivskyi
left a comment
There was a problem hiding this comment.
OK, this looks good as a good starting point. I think we will need some more things to make native parser a full replacement, but it is probably better to move in small increments. I will merge this now, release a new version, then we can continue discussion in the mypy PR.
This is the mypy counterpart of mypyc/ast_serialize#54 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>
Resolves #21
Tests are part of python/mypy#21260