twisted / twistedchecker

twistedchecker is a tool to automatically verify code against the Twisted coding standard.
MIT License
9 stars 13 forks source link

Use entry_points for twistedchecker command line tool. #97

Closed opalmer closed 9 years ago

opalmer commented 9 years ago

On Windows and sometimes other platforms calling pip install -e or python setup.py develop will not actually cause twistedchecker to end up on $PATH. On Windows this change ensures that you end up creating an executable that will run twisted checker in much the same way that bin/twistedchecker does.

opalmer commented 9 years ago

Hey, so while working on some other contributions I noticed that twistedchecker didn't work on Windows unless I manually created a cmd script to run the checker. I didn't remove the existing script in bin/ because I was not sure how else twistedcheck was already being used. It may be that having both the script and the entry point could cause conflicts (though it looks like it still works on Travis).

adiroiban commented 9 years ago

Many thanks for the changes!

I think that we should remove the script from bin and only use entry point to avoid confusing future developers.

Entry points can be also be used by 3rd party applications so we should only have entry points as the only way in which twistedcheker is called.

Instead of setup.py develop we can use setup.py install to check that the package is successfully installed ... but the twistedcheker -h command should be executed from another working directory... maybe from /tmp. What do you think?

Please update your branch based on my feedback.

Thanks!

opalmer commented 9 years ago

All that sounds good to me, updated the code to reflect the changes. The only difference I have is to cd to / rather than /tmp because /tmp may not always if the temp directory has changed.

I thought about removing the script initially but was unsure if something might be relying on it. The on

adiroiban commented 9 years ago

Why would anyone miss the old script? the script generated using entry_point should have the exact same functionality...

it might break when someone is using twistedchecker without installing it and the bin/twistedchecker no longer exists in the source package... but I think that this a bad practice.

I think that Twisted's twistedchecker builder (https://buildbot.twistedmatrix.com/builders/twistedchecker) will fail as it does not install the package

Also Twisted's twistedchecker is using the twistedchecker from master and not a released version.

To not break the Twisted builders I will try to fix the builder first and then will land it.

Your changes look good and I think they can be merged.

Thanks!

opalmer commented 9 years ago

Your changes look good and I think they can be merged. Thanks I'll keep a watch on this PR in case another change comes up after looking at the buildbot builder.

I didn't think most would miss the old script to be honest. Most projects that I've made this kind of change on in the past have never had issues with either keeping both or just keeping entry_points. I was more concerned about breaking buildbot in some way...I just didn't have a good way of testing it so I was being cautious heh.

adiroiban commented 9 years ago

ok... venv builder was applied on production. ... and 0.4.0 was already release.

I will merge it in trunk and will wait for @hawkowl to do a new release.

many thanks for your work!