Use Fields __all__ #70
Conversation
|
Thanks for doing this, happy to see this!! |
pylint_nautobot/use_fields_all.py
Outdated
| ) | ||
|
|
||
|
|
||
| class NautobotUseFieldsAll(BaseChecker): |
There was a problem hiding this comment.
Just making sure that we are purposeful, do we want to:
- Create one check for all types (serializer, forms, tables, etc)
- Create one check for each type, using mostly the same code?
There was a problem hiding this comment.
I am 50/50 on this. I see the benefit to have essentially N subclasses with an abstracted method on checking ancestor BUT that adds repetitive code.
There was a problem hiding this comment.
We just agreed to go with single checker for all types.
pylint_nautobot/use_fields_all.py
Outdated
| _META_CLASSES = ( | ||
| "nautobot.utilities.tables.BaseTable.Meta", | ||
| # TBD: Add support for other Meta classes | ||
| "nautobot.extras.forms.NautobotModelForm.Meta", |
There was a problem hiding this comment.
Could you please help me identify all classes that should be checked?
There was a problem hiding this comment.
This is the list / thoughts I cam up with:
- NautobotModelSerializer - Yes
- NautobotModelForm - Yes
- NautobotFilterSet - Capability should be added, but does not exist today
- NautobotBulkEditForm - Unclear, in floor app I see it defined, but others I do not, perhaps it is an implied all, in which case I think better to be explicit and convert to respect the all param as best practices)
- NautobotFilterForm - Not needed (can investigate other options for field_order)
- BaseTable - Worth investigation what can be done, but does not exist today and may not be applicable
There was a problem hiding this comment.
Added to _META_CLASSES.
snaselj
force-pushed
the
u/snaselj-napps-184-use-all
branch
2 times, most recently
from
70c9756
to
aa9796f
Compare
snaselj
force-pushed
the
u/snaselj-napps-184-use-all
branch
from
aa9796f
to
874f796
Compare
|
This PR is ready for review @whitej6 @cmsirbu @Kircheneer |
|
Adding a screenshot of failing rule.
|
pylint_nautobot/use_fields_all.py
Outdated
| # NautobotBulkEditForm - Unclear, in floor app I see it defined, but others I do not, perhaps it is an implied all, in which case I think better to be explicit and convert to respect the all param as best practices) | ||
| # NautobotFilterForm - Not needed (can investigate other options for field_order) |
There was a problem hiding this comment.
Perhaps better to move the uncertain ones to a new issue and discuss there what else could be added?
There was a problem hiding this comment.
FilterForm requires a change upstream before it can be added for __all__
BulkEditForm I do not believe this is applicable since this does not have an ancestor of django.forms.ModelForm
There was a problem hiding this comment.
Comments are removed.
|
All comments has been addressed, pull request is ready for review. |
Closes NaN
What's Changed
nb-use-fields-allchecker.To Do
fieldsnode value as a failing node.