Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#8606 enhancement closed fixed (fixed)

Merge setup.py and setup3.py

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

Description

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

maybe by refactoring it into something like https://gist.github.com/adiroiban/5b461f3ca96828a3036c99964d1d236e

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 https://github.com/twisted/twisted/compare/8606-rodrigc-setup-2

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

I saw that the setup args are defined both in setup.py and dist.py

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

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

I would also rename dist3.py to something like


Regarding the coverage for setup.py ... I can do coverage run setup.py and it will report as the setup.py being covered... but the point is to have something like coverage run trial test_setup.py to make sure that we have tests for the setup.py 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 setup3.py and setup.py. 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 setup.py 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 setup.py coverage issue... basically once this is merged setup.py 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 setup.py is the wrong way of fixing the setup.py coverage problem.


I am also requesting the review of this branch :) https://github.com/twisted/twisted/compare/8606-rodrigc-setup-2

Here is the coverage diff https://codecov.io/gh/twisted/twisted/compare/trunk...8606-rodrigc-setup-2

My branch is merging setup.py and setup3.py 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 dist3.py to _python3_port as it really contains just the list of ported modules.
  1. I have cleaned setup.py so that we define it only in dist.py ... previous some changes were in setup.py while others were in dist.py . I think that the current branch has a better design.
  1. In dist.py 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

Thanks!

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 setup.py 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.

Thanks!

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 setup.py and setup3.py merge and to also fix the coverage issue for setup.py.

I think that my cleanup branch is working on improving the coverge for setup.py and dist.py

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.

Thanks!

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 https://github.com/twisted/twisted/pull/337. 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 https://github.com/twisted/twisted/compare/8606-rodrigc-setup-2, 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 setup.py and setup3.py 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.