Skip to content

Conversation

@Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Jul 6, 2019

Description

At the cost of forcing to rename the old symbol when you create an old_name. Making the couple msgid/symbol a 1-1 relation removes legacy functions and complexity in order to ease implementation and optimize performance in the MessageStore. It will also permit to have multiple messages with the same old name for #1164.

Type of Changes

Type
🔨 Refactoring

@coveralls
Copy link

coveralls commented Jul 6, 2019

Coverage Status

Coverage increased (+0.02%) to 90.024% when pulling 4aa99db on Pierre-Sassoulas:message-store-refactor into 4bf6f03 on PyCQA:master.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the message-store-refactor branch 11 times, most recently from 43a7959 to 879b2d7 Compare July 7, 2019 20:50
@PCManticore
Copy link
Contributor

There's a lot to review here, just wanted to mention that I'm in the progress of reviewing it, but it's going to take a bit.

@Pierre-Sassoulas
Copy link
Member Author

Hi @PCManticore, thank you for taking the time to review this. I know it's huge, I could try to separate the style/lint issues from the main refactor if you want. I'm still working on simplifying the way the MessageDefinitionStore class works by using the new possibility of the MessageIdStore, so there will still be some changes even if they should be localized in the MessageDefinitionStore only. I also have performance tests results to provide once its done. I thougth of adding a regex check of the symbol so it's a string with dash and alphabetic non accentuated character only, but this will be cleaner if done later in another PR.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the message-store-refactor branch 2 times, most recently from 1c1e1a9 to 9a66bc9 Compare July 16, 2019 22:14
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jul 16, 2019

A rebase of the master branch broke the tests so I force pushed to go back to 0eeb6774 from 14 days ago. I hope the problem come from the master branch :D

Regarding performances, I tested by analysing the current pylint code, so it seems that results are not comparable because I call "is_enabled" more on the new pylint code (there is more class, with the new MessageId), I'll have to do a more stable test and take all function into account. Seems like it could be a little slower. Let's make #1164 works and then optimize performances later (I have some ideas, using msgid as hash instead of a MessageId for example)

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jul 17, 2019

@PCManticore, I'm going to take a look at the two failing tests in python3.5 this week, but apart from that, the pull request won't change anymore.

@Pierre-Sassoulas
Copy link
Member Author

@PCManticore do you want me to break this one into multiple smaller merge requests?

@Pierre-Sassoulas Pierre-Sassoulas changed the title Add a MessageId class to handle MessageDefinition msgid and symbol WIP - Add a MessageId class to handle MessageDefinition msgid and symbol Aug 3, 2019
@Pierre-Sassoulas Pierre-Sassoulas changed the title WIP - Add a MessageId class to handle MessageDefinition msgid and symbol WIP - Make the relation between msgid and symbol a 1 to 1 relation Aug 3, 2019
@Pierre-Sassoulas Pierre-Sassoulas changed the title WIP - Make the relation between msgid and symbol a 1 to 1 relation WIP - Make the relation between msgid and symbol 1:1 relation Aug 3, 2019
@Pierre-Sassoulas Pierre-Sassoulas changed the title WIP - Make the relation between msgid and symbol 1:1 relation WIP - Make the relation between msgid and symbol 1:1instead of 1:N Aug 3, 2019
@Pierre-Sassoulas
Copy link
Member Author

@PCManticore I'm going to simplify this and create other merge requests for clarity.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the message-store-refactor branch 2 times, most recently from b41617e to 4a264f6 Compare August 3, 2019 20:27
@Pierre-Sassoulas
Copy link
Member Author

@PCManticore I created #3040, #3041 and #3042 in order to make this one easier to review :)

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the message-store-refactor branch 2 times, most recently from 2dde2ef to a77a7b6 Compare August 4, 2019 18:34
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 4, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
@Pierre-Sassoulas Pierre-Sassoulas changed the title WIP - Make the relation between msgid and symbol 1:1instead of 1:N Make the relation between msgid and symbol 1:1instead of 1:N Aug 16, 2019
@Pierre-Sassoulas
Copy link
Member Author

@PCManticore, this one is ready for review. I can't make it significantly easier to review anymore, but I did not change the functional tests so the new message backend work as well as the one before. For now the performance are not better, but they will be after we merge #3013, see here for some performance analysis regarding this change.

@PCManticore
Copy link
Contributor

Thanks @Pierre-Sassoulas ! This looks good to go for me, does it depend on any other PR? Let me know if that's the case.

@Pierre-Sassoulas
Copy link
Member Author

Nice! It's mergeable as it is, then I'll update the two PR that depends on this one. We're getting closer! :)

@PCManticore PCManticore merged commit ac25a27 into pylint-dev:master Aug 19, 2019
@Pierre-Sassoulas Pierre-Sassoulas deleted the message-store-refactor branch August 19, 2019 16:24
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 19, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Sep 10, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Sep 10, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
PCManticore pushed a commit that referenced this pull request Sep 10, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request #2036 by Ashley Whetter.

See also #2075, #2077, #2262, #2654, #2805, #2809, #2844, #2992
and #3013 for full historical context.
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