Skip to content

Conversation

cis-muzahid
Copy link
Contributor

@cis-muzahid cis-muzahid commented Oct 29, 2021

Remove unneeded variable from addresses.py

Copy link
Member

@PeterSurda PeterSurda left a comment

Choose a reason for hiding this comment

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

remove test_addresses.py

@g1itch
Copy link
Collaborator

g1itch commented Nov 3, 2021

This is now about removing the kwarg. So you may just rebase those redundant commits:

$ git checkout test_address
$ git fetch upstream
$ git merge-base base58-nokwarg upstream/v0.6
1bf314545541f10a3284634f1dae3925cfc44262
$ git rebase -i 1bf314545541f10a3284634f1dae3925cfc44262

d 30286d1f ignore coverage files
pick 53196b85 cover two methods in coverage
s 876d9dbb remove passing alphabes using same file and fix issue
s d6070aae add more coverage
s 5ecafef6 add more coverage
s 034936e5 fixing code quality issue
s 03bd50ea revert changes
s f5b820b5 remove new test cases and restore address
s b94986e0 changes made older
s a71d041a remove import
s 627cf4ec changes done

$ git restore --staged .gitignore
$ git restore .gitignore
$ git rebase --continue

>> remove passing alphabet into encodeBase58 and decodeBase58 in addresses (do ^k ^k ^k ^k for the rest messages)

The result

@PeterSurda
Copy link
Member

@g1itch how about I add another check to limit the number of commits per PR to 3?

@g1itch
Copy link
Collaborator

g1itch commented Nov 4, 2021

@g1itch how about I add another check to limit the number of commits per PR to 3?

As for me it's a very strange decision. The argumentation is the same as I wrote in #1828: any change more complex than removing a couple of spaces may require more than 3 commits. #1389 or #1794 are not fit to such limit. When you're solving different aspects of the same problem it's better to do it in the separate commits in my opinion.

Currently the only wish for the project, I have, is the coherent and orderly development process: when each developer looks into the PRs of others, writes meaningful PR descriptions and commit messages, squashes the commits reverting each other. Thank god, at least everybody started to rebase their branches to v0.6 tip. I receive a dozen of notifications a day and try to look into each PR, but most of them are hard to read. The commit history is also barely readable. That's why I'm suggesting the squashing and rewording.

@PeterSurda
Copy link
Member

As for me it's a very strange decision. The argumentation is the same as I wrote in #1828: any change more complex than removing a couple of spaces may require more than 3 commits. #1389 or #1794 are not fit to such limit. When you're solving different aspects of the same problem it's better to do it in the separate commits in my opinion.

I want the developer to get meaningful automated feedback. If it can't be automated, then I need to do it manually, and that's error prone and time consuming.

Currently the only wish for the project, I have, is the coherent and orderly development process: when each developer looks into the PRs of others, writes meaningful PR descriptions and commit messages, squashes the commits reverting each other.

We're having a daily meeting where we discuss PRs, the problem is I can't think of everything everytime, and it gets more error prone when I have to review the same PR twenty times.

Thank god, at least everybody started to rebase their branches to v0.6 tip.

I wrote a script for that, see https://git.bitmessage.org/PeterSurda/termux_devenv/raw/branch/master/files/git-tools.sh
It checks all your open PRs and if it needs rebasing, rebases them. Set your repo to origin and upstream to upstream to get it working. Unfortunately another problem appeared, if it happens too frequently, github will hit the API rate limit and they refused to raise it for me even thought I'm on a paid account.

I receive a dozen of notifications a day and try to look into each PR, but most of them are hard to read. The commit history is also barely readable.

This is a fair point, the problem is this burdens me until the developers are trained properly.

That's why I'm suggesting the squashing and rewording.

That's a good suggestion but as I said I'd like to automate this check somehow.

@cis-muzahid cis-muzahid force-pushed the test_address branch 4 times, most recently from 58a98a5 to 512a7dd Compare November 11, 2021 07:44
@PeterSurda PeterSurda changed the title Test address Remove unneeded variable from addresses.py Nov 11, 2021
@PeterSurda PeterSurda marked this pull request as ready for review November 12, 2021 06:59
@PeterSurda PeterSurda merged commit c0f5e1b into Bitmessage:v0.6 Nov 18, 2021
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