Polish the qidenticon #1793

2021-07-27T17:53:35+02:00

g1itch commented

2021-07-27 17:53:35 +02:00

(Migrated from github.com)

This is taken from qt5-wip branch. The module is formatted to satisfy all the lints, and checked for compatibility with PyQt5 and python3. The actual switch to PyQt5 can be done by replacing one import:

--- a/src/qidenticon.py
+++ b/src/qidenticon.py
@@ -40,7 +40,7 @@ Returns an instance of :class:`QPixmap` which have generated identicon image.
 ``size`` specifies `patch size`. Generated image size is 3 * ``size``.
 """
 
-from PyQt4 import QtCore, QtGui
+from PyQt5 import QtCore, QtGui
 
 
 class IdenticonRendererBase(object):

I'm writing tests based on the comment in bitmessageqt.utils.

This is taken from qt5-wip branch. The module is formatted to satisfy all the lints, and checked for compatibility with PyQt5 and python3. The actual switch to PyQt5 can be done by replacing one import: ```diff --- a/src/qidenticon.py +++ b/src/qidenticon.py @@ -40,7 +40,7 @@ Returns an instance of :class:`QPixmap` which have generated identicon image. ``size`` specifies `patch size`. Generated image size is 3 * ``size``. """ -from PyQt4 import QtCore, QtGui +from PyQt5 import QtCore, QtGui class IdenticonRendererBase(object): ``` I'm writing tests based on [the comment](../blob/v0.6/src/bitmessageqt/utils.py#L20) in `bitmessageqt.utils`.

PeterSurda (Migrated from github.com) reviewed 2021-08-04 10:52:18 +02:00

PeterSurda (Migrated from github.com) left a comment

So far looks ok.

g1itch commented

2021-08-26 13:47:59 +02:00

(Migrated from github.com)

I tired of rebasing ): no time to finish the test

PeterSurda commented

2021-08-26 14:28:57 +02:00

(Migrated from github.com)

I tired of rebasing ): no time to finish the test

I have a script to help mass rebasing, I'll send it to you.

> I tired of rebasing ): no time to finish the test I have a script to help mass rebasing, I'll send it to you.

g1itch commented

2021-08-26 14:33:56 +02:00

(Migrated from github.com)

I tired of rebasing ): no time to finish the test

I have a script to help mass rebasing, I'll send it to you.

The problem is not about rebasing itself. I usually rebase work in progress branches if they have conflicts with the fresh merged code. Because of ongoing code grooming I forced to rebase them constantly because of the tiny changes in formatting.

> > I tired of rebasing ): no time to finish the test > > I have a script to help mass rebasing, I'll send it to you. The problem is not about rebasing itself. I usually rebase work in progress branches if they have conflicts with the fresh merged code. Because of ongoing code grooming I forced to rebase them constantly because of the tiny changes in formatting.

PeterSurda commented

2021-08-26 14:38:18 +02:00

(Migrated from github.com)

The problem is not about rebasing itself. I usually rebase work in progress branches if they have conflicts with the fresh merged code. Because of ongoing code grooming I forced to rebase them constantly because of the tiny changes in formatting.

I understand, it's difficult to move the development forward. Maybe if there was a way for you to indicate which files shouldn't be modified, or if you could separate changing them into another PR.

The code of this PR looks ok but the tests are skipped on buildbot. Give me a bit of time to get them working.

> The problem is not about rebasing itself. I usually rebase work in progress branches if they have conflicts with the fresh merged code. Because of ongoing code grooming I forced to rebase them constantly because of the tiny changes in formatting. I understand, it's difficult to move the development forward. Maybe if there was a way for you to indicate which files shouldn't be modified, or if you could separate changing them into another PR. The code of this PR looks ok but the tests are skipped on buildbot. Give me a bit of time to get them working.

g1itch commented

2021-08-26 14:39:30 +02:00

(Migrated from github.com)

Because of ongoing code grooming I forced to rebase them constantly because of the tiny changes in formatting.

I don't see a sense in the most of the recent commits merged, some of them make the code quality worse in my opinion.

> Because of ongoing code grooming I forced to rebase them constantly because of the tiny changes in formatting. I don't see a sense in the most of the recent commits merged, some of them make the code quality worse in my opinion.

PeterSurda commented

2021-08-26 14:43:04 +02:00

(Migrated from github.com)

I don't see a sense in the most of the recent commits merged, some of them make the code quality worse in my opinion.

Some of them were for python3 compatibility even though it's not directly showing up in current tests.

> I don't see a sense in the most of the recent commits merged, some of them make the code quality worse in my opinion. Some of them were for python3 compatibility even though it's not directly showing up in current tests.

g1itch commented

2021-08-26 14:43:14 +02:00

(Migrated from github.com)

The code of this PR looks ok but the tests are skipped on buildbot. Give me a bit of time to get them working.

There is how I made it in the github workflow: a1406f7

> The code of this PR looks ok but the tests are skipped on buildbot. Give me a bit of time to get them working. There is how I made it in the github workflow: a1406f7

g1itch commented

2021-08-26 14:47:43 +02:00

(Migrated from github.com)

I don't see a sense in the most of the recent commits merged, some of them make the code quality worse in my opinion.

Some of them were for python3 compatibility even though it's not directly showing up in current tests.

I don't think so. Some of them show that the author don't really understand the code and walks on a touch. Don't suppress the pylint warnings if them are not proven to be wrong. You can have the same result just turning off the pylint check. Many changes that you do now are caused by misunderstanding the pylint in the past, so code gets edited twice in the opposite direction.

> > I don't see a sense in the most of the recent commits merged, some of them make the code quality worse in my opinion. > > Some of them were for python3 compatibility even though it's not directly showing up in current tests. I don't think so. Some of them show that the author don't really understand the code and walks on a touch. Don't suppress the pylint warnings if them are not proven to be wrong. You can have the same result just turning off the pylint check. Many changes that you do now are caused by misunderstanding the pylint in the past, so code gets edited twice in the opposite direction.

PeterSurda commented

2021-08-26 14:48:32 +02:00

(Migrated from github.com)

There is how I made it in the github workflow: a1406f7

I want to do something like that, I expect some side effects on checkdeps, but I don't think it's that important now.

PS you can directly make a pull request against travis2bash.sh here: https://git.bitmessage.org/Bitmessage/buildbot-scripts

> There is how I made it in the github workflow: [a1406f7](https://github.com/Bitmessage/PyBitmessage/commit/a1406f7fc59af318b660d900e697a40520851794) I want to do something like that, I expect some side effects on `checkdeps`, but I don't think it's that important now. PS you can directly make a pull request against travis2bash.sh here: https://git.bitmessage.org/Bitmessage/buildbot-scripts

PeterSurda commented

2021-08-26 14:51:07 +02:00

(Migrated from github.com)

I don't think so. Some of them show that the author don't really understand the code and walks on a touch. Don't suppress the pylint warnings if them are not proven to be wrong. You can have the same result just turning off the pylint check. Many changes that you do now are caused by misunderstanding the pylint in the past, so code gets edited twice in the opposite direction.

The problem is, it's been taking too long. It's now 3.5 years of code quality work and it's barely halfway done. At the moment I would prefer to suppress warning which have complicated solutions so that I can decommission the old code quality checks and buildbot's checks will be green. Then it can be fixed later.

> I don't think so. Some of them show that the author don't really understand the code and walks on a touch. Don't suppress the pylint warnings if them are not proven to be wrong. You can have the same result just turning off the pylint check. Many changes that you do now are caused by misunderstanding the pylint in the past, so code gets edited twice in the opposite direction. The problem is, it's been taking too long. It's now 3.5 years of code quality work and it's barely halfway done. At the moment I would prefer to suppress warning which have complicated solutions so that I can decommission the old code quality checks and buildbot's checks will be green. Then it can be fixed later.

g1itch commented

2021-08-26 15:07:51 +02:00

(Migrated from github.com)

At the moment I would prefer to suppress warning which have complicated solutions so that I can decommission the old code quality checks and buildbot's checks will be green.

I like the old code quality checks more because they work with the changeset, not the whole repo.

> At the moment I would prefer to suppress warning which have complicated solutions so that I can decommission the old code quality checks and buildbot's checks will be green. I like the old code quality checks more because they work with the changeset, not the whole repo.

PeterSurda commented

2021-08-26 15:12:12 +02:00

(Migrated from github.com)

I like the old code quality checks more because they work with the changeset, not the whole repo.

I also like their utility, but I can't maintain and scale them. It's a hack that I did over a couple of weekends, it wasn't supposed to be deployed this long.

More recent buildbot now has improved differential checks, but I need to have some of my changes merged and tested before I can upgrade.

> I like the old code quality checks more because they work with the changeset, not the whole repo. I also like their utility, but I can't maintain and scale them. It's a hack that I did over a couple of weekends, it wasn't supposed to be deployed this long. More recent buildbot now has improved differential checks, but I need to have some of my changes merged and tested before I can upgrade.

PeterSurda (Migrated from github.com) approved these changes 2021-08-26 15:23:57 +02:00

PeterSurda (Migrated from github.com) left a comment

Code ok, tests finally execute, feel free to merge.

g1itch commented

2021-08-26 15:42:08 +02:00

(Migrated from github.com)

Oh, I forgot to add xvfbwrapper, because I have it in some helper branch...

This repo is archived. You cannot comment on pull requests.

No reviewers

PeterSurda