Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#8606 enhancement closed fixed (fixed)

Merge and

Reported by: Adi Roiban Owned by: GitHub <noreply@…>
Priority: normal Milestone:
Component: release management Keywords:
Cc: radix Branch:


there is a lot of code duplication.... and we should write a test for the itself

maybe by refactoring it into something like

Change History (11)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: radix added

comment:2 Changed 6 years ago by Craig Rodrigues

Keywords: review added

comment:3 Changed 6 years ago by Adi Roiban

Many thanks for the changes!

Please see my changes from the 8606-rodrigc-setup-2 branch

I tried to refactor the to use get_setup_args to just find out that this method is already defined in

I saw that the setup args are defined both in and

I think that we should move everything to and write test for the code from there... some parts of are already covered.

I think that is bad to have setup related declarations both in and

I would also rename to something like

Regarding the coverage for ... I can do coverage run and it will report as the being covered... but the point is to have something like coverage run trial to make sure that we have tests for the code.

Please commit in this branch you suggestion for the changes in tox.ini

comment:4 Changed 6 years ago by Craig Rodrigues

I don't like the direction that adiroiban is taking this in. The patch I had was simple and got the job done in terms of merging and adiroiban is making this more complicated than it needs to be, and is wasting time.

I'm going to ask adiroiban to stop reviewing this ticket, and I will wait to get feedback from another Twisted developer.

In terms of tox.ini, I will not modify it, but I think it is sufficient to do coverage run to achieve coverage. adiroiban can do that under a separate ticket.

comment:5 Changed 6 years ago by Adi Roiban

Please don't take it personal.

My branch was just a suggestion and not a review to this patch. This is why I have not removed the review keywords.

I have only asked you to check how I would like this cleanup to be made.

I think that my suggestion is a better way for fixing the coverage issue... basically once this is merged should be never touched again in other branches :)

I think that we are not on the same page in regard to the purpose of why we want to get the coverage report.

I think that adding coverage run is the wrong way of fixing the coverage problem.

I am also requesting the review of this branch :)

Here is the coverage diff

My branch is merging and but is also cleaning some code which is no longer used.

  1. I have removed admin/run-pyton3 scripts as I think that we no longer need them.
  1. I have renamed to _python3_port as it really contains just the list of ported modules.
  1. I have cleaned so that we define it only in ... previous some changes were in while others were in . I think that the current branch has a better design.
  1. In I have removed the functions which were declarative in nature and converted them to module level variables.
  1. I have simplified the way setup() is called and the way setup args are generated... they are now generated in a single place.
  1. build_scripts_twisted was removed as we no longer have scripts... this should have been done in #8491 but I don't want to revert that merge.
  1. getVersion was removed as it is not used


Last edited 6 years ago by Adi Roiban (previous) (diff)

comment:6 Changed 6 years ago by Craig Rodrigues

@adi I'm going to ask you to stop here, and move on to something else, and I will get someone else to review what I've submitted.

When I have tried to submit multiple fixes for in one PR, you told me to undo things, and now you are doing the same thing.

So, please stop, and work on something else.


comment:7 Changed 6 years ago by Adi Roiban

I think that you misunderstood my role in this ticket.

I have submitted my patch as a contributor and not as a reviewer.

My change will need to be reviewed by someone else just like for any other contribution... otherwise I would have just merged my changes without a review. I don't pretend that my changes are perfect. This is why I am asking for a review.

When I have created this ticket my intention was to have the and merge and to also fix the coverage issue for

I think that my cleanup branch is working on improving the coverge for and

I don't think that is wrong to have multiple people submitting solutions for the same problem.

I don't think that is wrong to disagree on a patch submitted my another contributor.

This is nothing personal. I was commenting only on this patch in particular and to your general contribution or to you as a person.

Someone reviewing this code will consider what patch to accept.

Please correct me if I am wrong.


comment:8 Changed 6 years ago by mark williams

I'd like both these changes to land. I appreciate rodrigc's point that adioroiban's changes address a larger set of issues than just setup/setup3 redundancy. They're still very important -- being able to test installation-related logic is critical.

First, I'd like to merge rodrigc's changes in They are low risk and provide immediate value, and should be mergeable now that we've loosened the coverage restrictions. Once they're rebased, I will OK their being merged by removing the "review" keyword from this ticket and ensuring it gets closed.

Then I'd like to see trunk merged into adiroiban's changes in, and have them attached to #8622. I will happily review them there so they can get merged.

Does that make sense to everyone? Let me know by dropping a comment into this ticket.

comment:9 Changed 6 years ago by Adi Roiban

Thanks for the review.

The same changes were proposed by Craig to be merged together with the console_scripts changes.

I have not accepted them in that ticket, not because I wanted to have them merge in a separate ticket exactly as they are, but because I considered that they need more work and I did not want to block the console_scripts changes.

As a common practice in Twisted, when we change some code we make sure that those changes are covered by tests, when is possible.

When I created the ticket I have not named it "merge and and write test" since for me is obvious that all changes should come with tests... and this ticket was a good opportunity to fix the coverage.

Also, if you merge something without tests, there is a risk that the follow up ticket for adding the test will never be followed up... I need incentives, and I fail to see the incentives in adding tests for a code which is already merged :)

Now, if you want to accept the changes without automated tests, I will not try to block you :)

Now, if you are confident that the changes are not breaking anything and that Amber will still be able to do the release by following the documented process, I will not try to block these changes.

From the wiki:

A reviewer reviews the completed work, and provides feedback: at least one good thing about the work, at least one area that needs improvement, and a judgement as to whether the good qualities ultimately outweigh the bad, i.e. whether the branch should be merged.

Now is your call what code you want to be merged and how :)

comment:10 Changed 6 years ago by GitHub <noreply@…>

Owner: set to GitHub <noreply@…>
Resolution: fixed
Status: newclosed

In 1671a08:

Error: Processor CommitTicketReference failed
 does not appear to be a Git repository. See the log for more information.

comment:11 Changed 6 years ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.