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

Adds a pysa quick start command: init-pya #506

Closed

Conversation

abishekvashok
Copy link
Contributor

@abishekvashok abishekvashok commented Oct 11, 2021

Adds a pysa quick command: init-pysa, that helps users setup a suitable
environment to run pysa. It handles commonly found issues in pysa and
creates the best possible environment while handling some of the edge
cases.

Test Plan

  • create (or enter into the directory of) a python project you'd like to run pysa on
  • install pyre from source (see installation docs) (suitably create an alias for the client)
  • run pyre init-pysa

Preview:
Preview

Signed-off-by: Abishek V Ashok abishekvashok@gmail.com
Fixes: MLH-Fellowship#80

@abishekvashok abishekvashok force-pushed the quickstart-automation branch 2 times, most recently from da4fb38 to 5b4894d Compare October 11, 2021 14:55
@abishekvashok
Copy link
Contributor Author

Can affirm linters and pyre are failing not due to these changes.

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@0xedward 0xedward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for working on this! I'm excited for this, since it'll help new users get set up with Pysa faster.

I had a few questions and suggestions

tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@abishekvashok
Copy link
Contributor Author

@0xedward made the requested changes! Thanks for taking time to review this!

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@0xedward 0xedward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes! Would you be able to fix some of the flake8 linter errors? :)

It looks like our open source config for flake8 is missing the lines our internal config of flake8 is catching. Would you be able to fix all the lines causing B950 lint error when running flake8 locally?

tools/pysa_quickstart/quickstart.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@abishekvashok
Copy link
Contributor Author

abishekvashok commented Oct 13, 2021

@0xedward removed the unused import. Sorry I missed that somehow. With the open source config, I tried including B950 explicitly with select and tried removing some conflicting rules, but couldn't reproduce any B950s but managed to make sure every line is below 88 in length (which is kinda what B950 specifies ig), but still we should mirror the internal config for flake8 and maybe should fix the linters as well in another PR.

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@0xedward
Copy link
Contributor

With the open source config, I tried including B950 explicitly with select and tried removing some conflicting rules, but couldn't reproduce any B950s but managed to make sure every line is below 88 in length (which is kinda what B950 specifies ig), but still we should mirror the internal config for flake8 and maybe should fix the linters as well in another PR.

@abishekvashok - ah no problem! That makes sense, because the Github actions lint run for flake8 also didn't show any of those errors. If there any other issues, I'll fix them and merge this PR.

Let's look into mirror our internal linter setup afterwards :)

@arthaud
Copy link
Contributor

arthaud commented Oct 14, 2021

Everything we are doing here seems like things most python projects would want to do. Is there a tool that could do this for us?
Also, are we expecting our users to git clone our repo and run that script? That seems higher entry than just pip install pyre + pyre init.
Do we want something like opam init that installs everything and puts something in the .bashrc? I find it annoying to have to enter a virtualenv everytime I want to use pyre/pysa. Also, it has the side effect of adding random python stuff in your environment. Most people don't want that.

@abishekvashok
Copy link
Contributor Author

Do we want something like opam init that installs everything and puts something in the .bashrc?

That is a good direction for us to tweak this towards.

@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@abishekvashok
Copy link
Contributor Author

Changed the script to be a command :)

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

client/pyre.py Outdated Show resolved Hide resolved
client/commands/v2/init_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/init_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/init_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/init_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/init_pysa.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@abishekvashok
Copy link
Contributor Author

@arthaud thank you for the suggestions, and sorry for messing up a bit. But it's fixed now :)

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@abishekvashok
Copy link
Contributor Author

Haven't touched any python files to cause pyre to break :)

client/commands/v2/initialize_pysa.py Show resolved Hide resolved
client/commands/v2/initialize_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/initialize_pysa.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@abishekvashok
Copy link
Contributor Author

@0xedward done :)

NB: Pyre is complaining about files not related to the PR.

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

client/commands/v2/initialize_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/initialize_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/initialize_pysa.py Outdated Show resolved Hide resolved
client/commands/v2/initialize_pysa.py Outdated Show resolved Hide resolved
client/pyre.py Outdated Show resolved Hide resolved
@abishekvashok abishekvashok changed the title Adds a pysa quick start script Adds a pysa quick start command: init-pya Oct 22, 2021
@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@abishekvashok
Copy link
Contributor Author

@arthaud done :)

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

client/pyre.py Outdated Show resolved Hide resolved
Adds a pysa quick start command that helps users setup a suitable
environment to run pysa. It handles commonly found issues in pysa and
creates the best possible environment while handling even the edge case
scenarios.

Signed-off-by: Abishek V Ashok <abishekvashok@gmail.com>
@facebook-github-bot
Copy link
Contributor

@abishekvashok has updated the pull request. You must reimport the pull request before landing.

@abishekvashok
Copy link
Contributor Author

I am amazed why flake8 couldn't detect the unused variable

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@0xedward merged this pull request in e7d5d75.

@gbleaney
Copy link
Contributor

Thanks for landing this! @abishekvashok would you also be able to update the quickstart docs to reference this?
https://pyre-check.org/docs/pysa-quickstart
https://github.com/facebook/pyre-check/blob/main/documentation/website/docs/pysa_quickstart.md

@abishekvashok
Copy link
Contributor Author

@gbleaney sure :)

@abishekvashok
Copy link
Contributor Author

@gbleaney sorry if it took a bit of time, but #519 :)

@gbleaney
Copy link
Contributor

@abishekvashok ha! I consider < 24 hours very fast :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fall 2021] Step 3: Automate Pysa Quickstart guide for new users to quickly initialize a environment for Pysa
5 participants