lfd / PaStA

The Patch Stack Analysis
GNU General Public License v2.0
33 stars 20 forks source link

Add patchwork interface #57

Closed rsarky closed 4 years ago

rsarky commented 4 years ago

This PR adds supoort to pull patches from patchwork. There are 2 ways PaStA can pull patches:

rralf commented 4 years ago

Thanks! I'll have a look at it tomorrow.

rralf commented 4 years ago

One thing: Could you take the adjustments of Config.py and load/write_index to a separate commits?

rsarky commented 4 years ago

One thing: Could you take the adjustments of Config.py and load/write_index to a separate commits?

Sure

rsarky commented 4 years ago

Updated

rsarky commented 4 years ago

Hi,

before I continue with my review: please remove unnecessary if len() checks, and take care of the indentation. In all cases, you can remove the else statement. While it's surely not a good idea, there's no need to disallow raw mboxes in combination with patchwork. It's not required for our use-case, but there's no reason to forbid it.

In all the cases, the else and the conditions based on len were introduced to disallow PaStA using Patchwork and public or raw mailboxes simultaneously. If we do want them to function together, I guess I can remove those checks

rralf commented 4 years ago

Hi, before I continue with my review: please remove unnecessary if len() checks, and take care of the indentation. In all cases, you can remove the else statement. While it's surely not a good idea, there's no need to disallow raw mboxes in combination with patchwork. It's not required for our use-case, but there's no reason to forbid it.

In all the cases, the else and the conditions based on len were introduced to disallow PaStA using Patchwork and public or raw mailboxes simultaneously. If we do want them to function together, I guess I can remove those checks

Yep, let's remove them.

rsarky commented 4 years ago

This patch breaks PaStA if we don't use a patchwork configuration:

File "/media/Datenfass/PaStA/pypasta/Config.py", line 199, in init mbox_patchwork = mbox['patchwork'] KeyError: 'patchwork'

Please take care to not break the system. Only address patchwork-related config items, if a patchwork configuration is being used.

The same thing would happen if we removed [mbox.raw] or [mbox.pubin] from the Config. This PR assumes that the config will by default contain the line [mbox.patchwork], this is the same way as pubins or raw mboxes are handled

rralf commented 4 years ago

This patch breaks PaStA if we don't use a patchwork configuration: File "/media/Datenfass/PaStA/pypasta/Config.py", line 199, in init mbox_patchwork = mbox['patchwork'] KeyError: 'patchwork' Please take care to not break the system. Only address patchwork-related config items, if a patchwork configuration is being used.

The same thing would happen if we removed [mbox.raw] or [mbox.pubin] from the Config. This PR assumes that the config will by default contain the line [mbox.patchwork], this is the same way as pubins or raw mboxes are handled

Oh, does it? Thanks for the bug report! :+1: Will fix this...

rsarky commented 4 years ago

Please split it up into two patches: The first one to change load_index, the second one that pulls out the method.

I didnt pull out the load_index method. That was done by you in a previous commit. Are you talking about the load_mbox_file method by any chance?

rsarky commented 4 years ago

I wonder why the indentation is breaking. I used to use vim, but now I switched to vscode that uses pep8 style for formatting

rsarky commented 4 years ago

Hi,

before I continue with my review: please remove unnecessary if len() checks, and take care of the indentation. In all cases, you can remove the else statement. While it's surely not a good idea, there's no need to disallow raw mboxes in combination with patchwork. It's not required for our use-case, but there's no reason to forbid it.

Done

rralf commented 4 years ago

This patch breaks PaStA if we don't use a patchwork configuration: File "/media/Datenfass/PaStA/pypasta/Config.py", line 199, in init mbox_patchwork = mbox['patchwork'] KeyError: 'patchwork' Please take care to not break the system. Only address patchwork-related config items, if a patchwork configuration is being used.

The same thing would happen if we removed [mbox.raw] or [mbox.pubin] from the Config. This PR assumes that the config will by default contain the line [mbox.patchwork], this is the same way as pubins or raw mboxes are handled

Oh, does it? Thanks for the bug report! +1 Will fix this...

Aah, we do not have a bug here. We set default values in resource's common.cfg:

https://github.com/lfd/PaStA-resources/blob/master/common/default.cfg

So the fix is simple: In future, we must add an empty [mbox.patchwork] entry to default.cfg. But we can postpone that for the moment.

rsarky commented 4 years ago

This patch breaks PaStA if we don't use a patchwork configuration: File "/media/Datenfass/PaStA/pypasta/Config.py", line 199, in init mbox_patchwork = mbox['patchwork'] KeyError: 'patchwork' Please take care to not break the system. Only address patchwork-related config items, if a patchwork configuration is being used.

The same thing would happen if we removed [mbox.raw] or [mbox.pubin] from the Config. This PR assumes that the config will by default contain the line [mbox.patchwork], this is the same way as pubins or raw mboxes are handled

Oh, does it? Thanks for the bug report! +1 Will fix this...

Aah, we do not have a bug here. We set default values in resource's common.cfg:

https://github.com/lfd/PaStA-resources/blob/master/common/default.cfg

So the fix is simple: In future, we must add an empty [mbox.patchwork] entry to default.cfg. But we can postpone that for the moment.

Yup makes sense

rralf commented 4 years ago

Rebased your work and pushed it to wip-patchwork-integration. Will test it now.

rsarky commented 4 years ago

Hi,

just successfully run your script. There are still some issues, we receive duplicate messages. Let's talk about that in a short video conference. Will send you an email tomorrow.

Thanks Ralf

Sure

rsarky commented 4 years ago

Added some error handling. Tested it out by stopping the patchwork server while the update was running. Works!

Added it to this commit

rralf commented 4 years ago

Looks good now! Thanks, merged to next and wip-patchwork-integration.