Skip to content
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

bpo-43950: Implement fine grained error locations for interactive mode #92827

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

thatbirdguythatuknownot
Copy link
Contributor

@thatbirdguythatuknownot thatbirdguythatuknownot commented May 15, 2022

Original issue number: #88116
Most of the implementation comes from #27117. Changes include new fields in the compiler struct (.c_source) and the PyCodeObject object (.co_source) to overcome the problem in the old PR by storing the source locally instead of globally. A new source argument was also added to _PyAST_Compile() to pass the source from the parser to the compiler.

@pablogsal
Copy link
Member

@pablogsal pablogsal commented May 16, 2022

I'm confused. The original PR #27117 is still open, even if it is a draft and the original design questions already remain. Why have you opened another PR? Have you contacted @isidentical first to take his code/approach?

We still need to decide if this is a good idea because this has drawbacks, which is one of the reasons we didn't pursue this originally.

@thatbirdguythatuknownot
Copy link
Contributor Author

@thatbirdguythatuknownot thatbirdguythatuknownot commented May 16, 2022

I'm confused. The original PR #27117 is still open, even if it is a draft and the original design questions already remain. Why have you opened another PR? Have you contacted @isidentical first to take his code/approach?

We still need to decide if this is a good idea because this has drawbacks, which is one of the reasons we didn't pursue this originally.

Alright, so I have to close this one?

@pablogsal
Copy link
Member

@pablogsal pablogsal commented May 16, 2022

Alright, so I have to close this one?

Well, you don't have to but the aspects and questions I have mentioned still stand. Particularly and most importantly, if you have taken code directly from #27117 then you need first to have the author's permission, otherwise, we won't be able to merge this even if we wanted.

@isidentical
Copy link
Sponsor Member

@isidentical isidentical commented May 16, 2022

My PR can be used as a baseline, no worries on that end (though I have some reservations about the implementation, especially the new code field).

@pablogsal
Copy link
Member

@pablogsal pablogsal commented May 16, 2022

I have some reservations about the implementation, especially the new code field).

Yeah, that's what I dislike the most and I think this may make it a no-go. At the end of the day the reason we left it there was because the extra overhead and complexity was not worth the advantages as other reprls like Ipython will not even benefit from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants