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

Add no namedtuple rule #136

Merged
merged 9 commits into from
Sep 25, 2020
Merged

Conversation

lukuang
Copy link
Contributor

@lukuang lukuang commented Sep 25, 2020

Summary

Introduce a rule that enforces the use of dataclass instead of NamedTuple for better customization and inheritance support

Test Plan

unitests

@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 Sep 25, 2020
@lukuang
Copy link
Contributor Author

lukuang commented Sep 25, 2020

Not sure about the test errors here. Can anyone help me?

@isidentical
Copy link
Contributor

I don't know how much community feedback was involved with this rule, but IMHO it is a terrible idea. namedtuple's are totally different than dataclasses on different use cases (including performance, usage etc). I'm totally -1 on this.

@jimmylai
Copy link
Contributor

Not sure about the test errors here. Can anyone help me?

@lukuang if you check the error message on Github builds, you'll find this:
ERROR: /Users/runner/work/Fixit/Fixit/fixit/rules/no_namedtuple.py Imports are incorrectly sorted and/or formatted.
The code format doesn't comply and you can fix it by running tox -e autofix locally.

@jimmylai
Copy link
Contributor

I don't know how much community feedback was involved with this rule, but IMHO it is a terrible idea. namedtuple's are totally different than dataclasses on different use cases (including performance, usage etc). I'm totally -1 on this.

Rules in Fixit doesn't mean they're standard Python convention that needs to be applied everywhere.
Rules can be disabled in config file via the block_list_rules.
https://fixit.readthedocs.io/en/latest/getting_started.html#Configuration-File

We found this convention is useful for us for reasons including consistency, performance and usability. So we built this rule.
PEP-557 explains some NamedTuple caveats that can be avoided by dataclass. This is a reference of performance benchmark.
https://medium.com/@jacktator/dataclass-vs-namedtuple-vs-object-for-performance-optimization-in-python-691e234253b9

Comment on lines 9 to 10
from fixit.common.base import CstLintRule
from fixit.common.utils import InvalidTestCase as Invalid, ValidTestCase as Valid
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this works, the convention is to import those helpers from fixit to be simpler and consistent to other rules.
https://fixit.readthedocs.io/en/latest/test_a_lint_rule.html#Writing-Unit-Tests

fixit/rules/no_namedtuple.py Outdated Show resolved Hide resolved
fixit/rules/no_namedtuple.py Outdated Show resolved Hide resolved
fixit/rules/no_namedtuple.py Show resolved Hide resolved
fixit/rules/no_namedtuple.py Outdated Show resolved Hide resolved
@jimmylai
Copy link
Contributor

@lukuang thanks for contributing!
I updated the message and docstring a bit to make it more clear and use RST formats. Let me know how do you like it.

@lukuang
Copy link
Contributor Author

lukuang commented Sep 25, 2020

@lukuang thanks for contributing!
I updated the message and docstring a bit to make it more clear and use RST formats. Let me know how do you like it.

LGTM! Thanks @jimmylai, it is more clear now. Please go ahead and 🚢

@jimmylai jimmylai merged commit efade0a into Instagram:master Sep 25, 2020
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.

4 participants