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

Expose ipc_main with arguments instead of argparse #175

Merged
merged 9 commits into from
Mar 4, 2021

Conversation

aehernandez
Copy link
Contributor

Summary

Refactored ipc_main into run_ipc which takes in arguments instead of relying on argparse. ipc_main still exists for backwards compatibility.

Test Plan

Added a test verifying that

  1. The new function runs on a single path and returns output as expected
  2. The new function still supports reporting on multiple input paths

Ran tox -e py38 to verify tests were running and green.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 25, 2021
@codecov-io
Copy link

codecov-io commented Feb 25, 2021

Codecov Report

Merging #175 (c572dcd) into master (ee0c2c9) will increase coverage by 0.84%.
The diff coverage is 92.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   85.04%   85.88%   +0.84%     
==========================================
  Files          89       90       +1     
  Lines        3623     3691      +68     
==========================================
+ Hits         3081     3170      +89     
+ Misses        542      521      -21     
Impacted Files Coverage Δ
fixit/cli/__init__.py 76.34% <40.00%> (+20.16%) ⬆️
fixit/cli/tests/test_ipc.py 100.00% <100.00%> (ø)
fixit/cli/tests/test_lint_opts.py 100.00% <100.00%> (ø)
fixit/rule_lint_engine.py 100.00% <0.00%> (+4.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee0c2c9...c572dcd. Read the comment docs.

@aehernandez aehernandez force-pushed the master branch 5 times, most recently from 4a96daf to 68e21db Compare February 25, 2021 05:09
Comment on lines 243 to 262
def ipc_main(opts: LintOpts) -> IPCResult:
"""
Like `run_ipc` instead this function expects arguments to be collected through argparse.
This IPC helper takes paths of source files from either a path file (with @paths arg)
or a list of paths as args.

Returns an IPCResult object.
"""
parser = argparse.ArgumentParser(
description="Runs Fixit lint rules and print results as console output.",
fromfile_prefix_chars="@",
parents=[get_multiprocessing_parser()],
)
parser.add_argument("paths", nargs="*", help="List of paths to run lint rules on.")
parser.add_argument("--prefix", help="A prefix to be added to all paths.")
args: argparse.Namespace = parser.parse_args()

return run_ipc(
opts=opts, paths=args.paths, prefix=args.prefix, workers=args.workers
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's deprecate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Added a deprecation warning using python's standard warnings.warn functionality.

Copy link
Member

@mvismonte mvismonte left a comment

Choose a reason for hiding this comment

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

Looks good!

@mvismonte mvismonte merged commit 2a8a228 into Instagram:master Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants