Added --name argument #131

acharles7 · 2020-09-17T07:10:54Z

Summary

Refer #119

Added --name argument to fixit's add_new_rule CLI
python -m fixit.cli.add_new_rule --path fixit/rules/my_rule.py --name abcd

--file = fixit/rules/my_rule.py
--name = abcd

The file will be created at fixit/rules/my_rule.py with class name abcd as AbcdRule

class AbcdRule(CstLintRule):
    """
    docstring or new_rule description
    """

    MESSAGE = "Enter rule description message"

    VALID = [Valid("'example'")]

    INVALID = [Invalid("'example'")]

Added snake_to_camelcase function to fixit/cli/utils.py
Added documentation to docs/source/build_a_lint_rule.ipynb

Test Plan

acharles7 · 2020-09-17T07:24:07Z

Please have a look at the docs builds error.
We have to avoid using In[2] to write in examples.py

acharles7 · 2020-09-18T16:18:18Z

@jimmylai Can you please look into it?

isidentical

Just suggestion, but how would it look using the path (like chained_isinstance_check.py) when there is no name supplied (convert the path's stem into ChainedIsinstanceCheck)

isidentical · 2020-09-18T16:44:52Z

fixit/cli/add_new_rule.py

-def create_rule_file(file_path: Path) -> None:
+def is_valid_name(name: str) -> str:
+    """Check for valid rule name """
+    if name.endswith(("Rule", "Rules", "rule", "rules")):


Suggested change

if name.endswith(("Rule", "Rules", "rule", "rules")):

if name.casefold().endswith(("rule", "rules")):

isidentical · 2020-09-18T16:46:14Z

fixit/cli/add_new_rule.py

+def is_valid_name(name: str) -> str:
+    """Check for valid rule name """
+    if name.endswith(("Rule", "Rules", "rule", "rules")):
+        raise NameError("Please enter rule name without the suffix, `rule` or `Rule`")


It would be more consistent if you use single quotes instead of backticks (just like KeyError etc.)

Also, NameError doesn't suit well, maybe use ValueError?

isidentical · 2020-09-18T16:48:31Z

fixit/cli/add_new_rule.py

+    """Check for valid rule name """
+    if name.endswith(("Rule", "Rules", "rule", "rules")):
+        raise NameError("Please enter rule name without the suffix, `rule` or `Rule`")
+    return snake_to_camelcase(name) if name else ""


You can omit if name else "" since snake_to_camelcase with empty string would result with an empty string.

acharles7 · 2020-09-18T18:09:09Z

Just suggestion, but how would it look using the path (like chained_isinstance_check.py) when there is no name supplied (convert the path's stem into ChainedIsinstanceCheck)

Done. Nice idea
Thanks

isidentical · 2020-09-18T18:12:40Z

docs/source/build_a_lint_rule.ipynb

  }
 },
 "nbformat": 4,
 "nbformat_minor": 4
-}
+}


Missing newline at the end

jimmylai · 2020-09-19T23:31:07Z

Please have a look at the docs builds error.
We have to avoid using In[2] to write in examples.py

For some reason, this error just show up recently.
In[2] is a way to access the content of cell number 2 in the notebook to reuse the code example.
tox -e docs run successfully in my local dev. It seems specific to the CI build.
Are you able to reproduce this error locally?

acharles7 · 2020-09-20T00:16:28Z

Please have a look at the docs builds error.
We have to avoid using In[2] to write in examples.py

For some reason, this error just show up recently.
In[2] is a way to access the content of cell number 2 in the notebook to reuse the code example.
tox -e docs run successfully in my local dev. It seems specific to the CI build.
Are you able to reproduce this error locally?

I am also getting this error in local dev.

acharles7 · 2020-09-20T00:19:50Z

We can use something like this.

file_content = """\
class C:
...content of In[2]
"""

then write file_content to examples.py

jimmylai · 2020-09-20T00:47:04Z

docs/source/getting_started.ipynb

+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {
+    "nbsphinx": "hidden"
+   },
+   "outputs": [],
+   "source": [
+    "file_content = \"\"\"\\\n",
+    "from typing import Dict\n",
+    "\n",
+    "\n",
+    "class C(object):\n",
+    "    attr = \"ab\" \"cd\" \"ef\" \"gh\"\n",
+    "\n",
+    "    def method(self) -> Dict[int, str]:\n",
+    "        filtered_char = []\n",
+    "        for char in self.attr:\n",
+    "            if char is not \"a\":\n",
+    "                filtered_char.append(char)\n",
+    "\n",
+    "        index_to_char = dict([(idx, char) for idx, char in enumerate(filtered_char)])\n",
+    "        return index_to_char\n",
+    "\"\"\""
+   ]
+  },


Copy the example code is not ideal. It'll make the future updated less easy.

I've found a workaround in #133

I'll revert it. Thanks

jimmylai · 2020-09-20T00:55:57Z

docs/source/build_a_lint_rule.ipynb

+    "- ``--path`` is used to create rule file at path given in ``--path``, Default is ``fixit/rules/new.py``\n",
+    "- ``--name`` is used to assign the name of the rule and should be in snake case, Default is name of the rule file if ``path`` provided else `new`. Otherwise, considers the value specified in ``--name``\n",


Suggested change

"- ``--path`` is used to create rule file at path given in ``--path``, Default is ``fixit/rules/new.py``\n",

"- ``--name`` is used to assign the name of the rule and should be in snake case, Default is name of the rule file if ``path`` provided else `new`. Otherwise, considers the value specified in ``--name``\n",

"- ``--path`` is used to create rule file at path given in ``--path``, defaults to ``fixit/rules/new.py``\n",

"- ``--name`` is used to assign the name of the rule and should be in snake case, defaults to the rule file name if ``path`` provided else `new`. Otherwise, considers the value specified in ``--name``\n",

jimmylai · 2020-09-20T00:59:44Z

fixit/cli/add_new_rule.py

+        "--name",
+        type=str,
+        default="",
+        help="Name of the rule, Default is New",


Suggested change

help="Name of the rule, Default is New",

help="Name of the rule, defaults to `New`",

jimmylai

Other than comments, LGTM.
Thanks for contributing!

Added name argument

ffc317d

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 Sep 17, 2020

acharles7 changed the title ~~Added --name argument to~~ Added --name argument Sep 17, 2020

isidentical reviewed Sep 18, 2020

View reviewed changes

acharles7 added 2 commits September 18, 2020 10:46

Added stem functionality

076124c

Docs changes

cb40ce9

isidentical reviewed Sep 19, 2020

View reviewed changes

Added new line

8e917f3

jimmylai reviewed Sep 20, 2020

View reviewed changes

acharles7 force-pushed the name-argument branch from 2726995 to b4ada29 Compare September 20, 2020 00:54

jimmylai reviewed Sep 20, 2020

View reviewed changes

jimmylai approved these changes Sep 20, 2020

View reviewed changes

Comment changes

3772a2f

acharles7 force-pushed the name-argument branch from b4ada29 to 3772a2f Compare September 20, 2020 01:47

Comments updated

cd48ebb

jimmylai merged commit aaa3446 into Instagram:master Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added --name argument #131

Added --name argument #131

acharles7 commented Sep 17, 2020 •

edited

Loading

acharles7 commented Sep 17, 2020

acharles7 commented Sep 18, 2020

isidentical left a comment

isidentical Sep 18, 2020

acharles7 Sep 18, 2020

isidentical Sep 18, 2020

isidentical Sep 18, 2020

isidentical Sep 18, 2020

acharles7 Sep 18, 2020

acharles7 commented Sep 18, 2020

isidentical Sep 18, 2020

acharles7 Sep 20, 2020

jimmylai commented Sep 19, 2020

acharles7 commented Sep 20, 2020

acharles7 commented Sep 20, 2020

jimmylai Sep 20, 2020 •

edited

Loading

acharles7 Sep 20, 2020

jimmylai Sep 20, 2020

acharles7 Sep 20, 2020

jimmylai Sep 20, 2020

acharles7 Sep 20, 2020

jimmylai left a comment

	if name.endswith(("Rule", "Rules", "rule", "rules")):
	if name.casefold().endswith(("rule", "rules")):

		"- ``--path`` is used to create rule file at path given in ``--path``, Default is ``fixit/rules/new.py``\n",
		"- ``--name`` is used to assign the name of the rule and should be in snake case, Default is name of the rule file if ``path`` provided else `new`. Otherwise, considers the value specified in ``--name``\n",

	help="Name of the rule, Default is New",
	help="Name of the rule, defaults to `New`",

Added --name argument #131

Added --name argument #131

Conversation

acharles7 commented Sep 17, 2020 • edited Loading

Summary

Test Plan

acharles7 commented Sep 17, 2020

acharles7 commented Sep 18, 2020

isidentical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acharles7 commented Sep 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylai commented Sep 19, 2020

acharles7 commented Sep 20, 2020

acharles7 commented Sep 20, 2020

jimmylai Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylai left a comment

Choose a reason for hiding this comment

acharles7 commented Sep 17, 2020 •

edited

Loading

jimmylai Sep 20, 2020 •

edited

Loading