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

Apps Refactor: Refactor all imports to use module namespaces #204

Open
2 of 8 tasks
jathanism opened this issue Mar 26, 2021 · 2 comments · Fixed by #2794
Open
2 of 8 tasks

Apps Refactor: Refactor all imports to use module namespaces #204

jathanism opened this issue Mar 26, 2021 · 2 comments · Fixed by #2794
Labels
epic A collection issues ultimately aligning to the same goal/outcome. type: housekeeping Changes to the application which do not directly impact the end user

Comments

@jathanism
Copy link
Contributor

jathanism commented Mar 26, 2021

Proposed Changes

Currently in the code base there are many examples of large imports into the global namespace. For example, here is a sample of forms-related imports from nautobot/extras/forms.py:

from django import forms

from nautobot.utilities.forms import (
    add_blank_choice,
    APISelectMultiple,
    BootstrapMixin,
    BulkEditForm,
    BulkEditNullBooleanSelect,
    ColorSelect,
    CSVModelChoiceField,
    CSVModelForm,
    CSVMultipleContentTypeField,
    DateTimePicker,
    DynamicModelChoiceField,
    DynamicModelMultipleChoiceField,
    JSONField,
    MultipleContentTypeField,
    SlugField,
    StaticSelect2,
    StaticSelect2Multiple,
    BOOLEAN_WITH_BLANK_CHOICES,
)

I'm proposing we move to this pattern:

from django import forms as djforms  # Django forms module
from nautobot.utilities import forms  # Our utilities.forms module
from nautobot.dcim import models as dcim_models  # DCIM models
    
 # In code..
class SomeForm(forms.CustomFieldModelForm, djforms.ModelForm):
    name = djforms.CharField(required=False)
    device = forms.CSVModelChoiceField(queryset=dcim_models.Device.objects.all())

    class Meta:
        model = dcim_models.Device

Justification

The current pattern is not only is unwieldy, but complicates merges, rebases, and refactors and in some cases can result in circular imports.

This way it's clear what is coming from Django forms and what is coming from internal forms modules, and any time something new is needed or used from nautobot.utilities.forms it doesn't need to be explicitly imported.

This also applies to more than just forms, but I was just selecting forms as a singular example of imports for a single module.

TODO

Related

@jathanism jathanism added type: housekeeping Changes to the application which do not directly impact the end user status: near-term labels Mar 26, 2021
@bryanculver
Copy link
Member

We should get this codified into best practices and work towards this in every PR. If you touch it, update it.

@jathanism jathanism added this to the v2.0.0 milestone Jul 21, 2022
@bryanculver bryanculver mentioned this issue Nov 3, 2022
3 tasks
@bryanculver bryanculver changed the title Refactor all imports to use module namespaces Apps Refactor: Refactor all imports to use module namespaces Nov 3, 2022
@gsnider2195 gsnider2195 self-assigned this Nov 9, 2022
@gsnider2195 gsnider2195 removed their assignment Nov 18, 2022
@gsnider2195
Copy link
Contributor

gsnider2195 commented Nov 22, 2022

Script to automate refactoring of imports

refactor_imports.zip

@bryanculver bryanculver added the epic A collection issues ultimately aligning to the same goal/outcome. label Feb 1, 2023
@bryanculver bryanculver removed this from the v2.0.0 milestone Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic A collection issues ultimately aligning to the same goal/outcome. type: housekeeping Changes to the application which do not directly impact the end user
Projects
No open projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

4 participants