Bitmessage/PyBitmessage-2024-12-12
Archived
2
0
Fork 0
Code Issues 168 Pull Requests 53 Actions Packages Projects Releases 5 Wiki Activity

Polish the qidenticon #1793

Merged
g1itch merged 4 commits from qidenticon into v0.6 2021-08-26 15:43:08 +02:00
Conversation 13 Commits 4 Files Changed 3 +125 -35
g1itch commented 2021-07-27 17:53:35 +02:00 (Migrated from github.com)
Copy Link

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.

So far looks ok.
g1itch commented 2021-08-26 13:47:59 +02:00 (Migrated from github.com)
Copy Link

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

I tired of rebasing ): no time to finish the test
PeterSurda commented 2021-08-26 14:28:57 +02:00 (Migrated from github.com)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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)
Copy Link

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.

Code ok, tests finally execute, feel free to merge.
g1itch commented 2021-08-26 15:42:08 +02:00 (Migrated from github.com)
Copy Link

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

Oh, I forgot to add xvfbwrapper, because I have it in some helper branch...
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
PeterSurda
Labels
Clear labels   
bug

Something isn't working as it's supposed to

  
build

Related to building or build system

  
dependencies

Pull requests that update a dependency file

  
developers

The issue is relevant mainly for developers rather than users

  
documentation

   
duplicate

   
enhancement

New feature

  
formatting

Code or UI text formatting

  
invalid

   
legal

   
mobile

Frontend for mobile devices (kivy)

  
obsolete

   
packaging

   
performance

   
protocol

Protocol change

  
question

   
refactoring

   
regression

Some issue, fixed in the past, appears again

  
security

   
test

   
translation

   
usability

   
wontfix
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2024-12-12#1793
No description provided.
Delete Branch "qidenticon"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?