Skip to content

Conversation

@zzzz-qq
Copy link
Contributor

@zzzz-qq zzzz-qq commented Apr 18, 2019

Lazy load unicode names when they are used first time rather than when ipython startup.

@Carreau
Copy link
Member

Carreau commented Apr 19, 2019

@LucianaMarques that's your code, you might want to review it;

But otherwise that's looks good to me.

@LucianaMarques
Copy link
Contributor

Hey there, sorry for the late reply.

If tests are working and this will improve performance, I don't see why not accept it :) That's actually really nice improvement, thanks!

I would like though that you provide us more info on the profiling you did, how you did it, etc. I'm not used to profiling yet and I would certainly like to learn more about it.

I am doing some work in the completer class, if you have some more suggestions I would love to hear it.

Copy link
Contributor

@LucianaMarques LucianaMarques left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a review on your pull request. Please take some time to consider my comments and let's discuss them :)

Also, I like to put descriptions in my pull requests, even if it's just something like "closes issue number #x". It's important to keep things documented in my opinion.

@Carreau
Copy link
Member

Carreau commented Apr 25, 2019

Hey there, sorry for the late reply.

No need to apologize, we are all busy.
Those were really good comment and questions.

I would like though that you provide us more info on the profiling you did, how you did it, etc. I'm not used to profiling yet and I would certainly like to learn more about it.

I don't think much profiling is necessary. It won't be "faster" in all cases, it will mostly delay the computation of the unicodes mapping until actually requested.

This mean that the startup of IPython will be a bit faster, but the first press on <tab> that needs completion will be a bit slower.

@LucianaMarques
Copy link
Contributor

LucianaMarques commented Apr 25, 2019

I have one more code comment but i'm on my phone now, please wait before merging just a little bit.

@Carreau
Copy link
Member

Carreau commented Apr 25, 2019

I have one more code comment but i'm on my phone now, please wait before merging just a little bit.

Don't worry plenty of time we'll wait for your approval.

@LucianaMarques
Copy link
Contributor

Don't worry plenty of time we'll wait for your approval.

Thanks! Just to sum up, only exclude the comment I mentioned above and add some description to the PR, and in my opinion it's ready to be merged.

@zzzz-qq zzzz-qq force-pushed the lazy-load-unicode-names-dev branch from 93ed931 to c69876f Compare April 26, 2019 03:46
@zzzz-qq
Copy link
Contributor Author

zzzz-qq commented Apr 26, 2019

Hey there, sorry for the late reply.

No need to apologize, we are all busy.
Those were really good comment and questions.

I would like though that you provide us more info on the profiling you did, how you did it, etc. I'm not used to profiling yet and I would certainly like to learn more about it.

I don't think much profiling is necessary. It won't be "faster" in all cases, it will mostly delay the computation of the unicodes mapping until actually requested.

This mean that the startup of IPython will be a bit faster, but the first press on <tab> that needs completion will be a bit slower.

@Carreau Really appreaciate your patience and kindness.

@Carreau Carreau added this to the 7.6 milestone Apr 26, 2019
@Carreau
Copy link
Member

Carreau commented Apr 26, 2019

Really appreciate your patience and kindness.

Thanks much appreciated; I wish I had more time to mentor and guide others.
Thanks to you for taking the time to send a pull-request, and allow us to discuss it; I know it can be frustrating to wait.

I just released 7.5 yesterday so this did not make the cut, but I'm hopping to get that in 7.6 next month.

@LucianaMarques
Copy link
Contributor

LucianaMarques commented Apr 28, 2019

@Carreau are we wainting for 7.6 to merge this PR? I don't have merge permission, so I was just wandering why you didn't merge this yet.

@Carreau
Copy link
Member

Carreau commented May 10, 2019

Thanks; I have some time to look at ongoing PRs, merging this.

Thanks both !

@Carreau Carreau merged commit 380db0d into ipython:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants