Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to use as command #287

Merged
merged 7 commits into from Nov 15, 2018

Conversation

4 participants
@tkmru
Copy link
Contributor

commented Nov 8, 2018

This make it possible to use as twint command. It will be more useful.

@tkmru tkmru force-pushed the tkmru:twint-command branch from f85fd67 to 8ff3ef1 Nov 8, 2018

@pielco11

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

Is this cross-platform?

@tkmru

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

I used scripts keywords in setuptools for using as command. It works in cross-platform.

@haccer

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

Thanks @tkmru! This is a good idea. Can you make changes to Twint.py and setup.py so we're using entry_points instead of scripts?

@tkmru tkmru force-pushed the tkmru:twint-command branch from 8226057 to 888b294 Nov 12, 2018

@tkmru

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

@haccer Since Twint.py is not included in twint package, I think it is difficult to specify entry_points. Is not it good to use scripts?

@haccer

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

entry_points is preferred. We should make adjustments to move Twint.py into the package.

@tkmru

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

@haccer I moved Twint.py into the package, changed to use entry_points instead of scripts.

@tkmru tkmru force-pushed the tkmru:twint-command branch from 586afd3 to ea8b678 Nov 12, 2018

@haccer

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Alright, I will probably won’t be available to verify this until next Sunday.

@pielco11, before merging can you make sure the behavior works as intended. Thanks.

@pielco11

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

It seems to be working as command but not via CLI since main() is not called.
I did not try with other OS than Ubuntu, yet.

@tkmru

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

sorry.

But is it necessary to use it as a single script from the CLI? It can be used as a command.
twint package is imported in twint.py, it is difficult to solve the path dependency.

If you want to be compatible with both being usable as a command and working as a single script, I think it is better to move twint.py outside twint directory and use scripts instead of entry_points.

@pielco11

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Usually I play with CLI when I add new arguments for features, but actually I just need to change twint/twint.py in the directory where the package is installed to achieve that, so I do agree that CLI is not necessary

I'll test with other OS as soon as possible

@pielco11

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

A little change that me and another member were talking about is: why not use some imports in twint/twint.py instead of importing the entire module? It's more "package-consistent". Plus importing the entire module will import twint.py itself, which is not needed.

Also we propose to change twint/twint.py to twint/cli.py to make the name more consistent

Plus this needs to be changed as well

ap = argparse.ArgumentParser(prog="Twint.py",

twint instead of Twint.py since the first will be the entry-point

@tkmru

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@pielco11 I fixed them.

@haccer

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

I had fun testing this. I think this is fine to Squash and merge. Thoughts @pielco11?

@pielco11

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

@haccer haccer merged commit 99bdad3 into twintproject:master Nov 15, 2018

@haccer

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

Thank you @tkmru!

@jvanalst

This comment has been minimized.

Copy link

commented Nov 20, 2018

The error message is wrong when you run it through the CLI, FYI.

vanalsti@~$ twint -s "Donald Trump" --verified --users
usage: python3 twint [options]
twint: error: unrecognized arguments: --users

Not sure why --users isn't recognized since it's in the examples, but it really shouldn't be telling me to use "python3 twint".

@pielco11

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

@jvanalst

This comment has been minimized.

Copy link

commented Nov 21, 2018

I opened a new issue to deal with the users flag, since I don't want to derail this on.

Still twint CLI shouldn't be telling users to user python3 twint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.