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

Decouple url_base and view_name #67

Open
SamuelJennings opened this issue Oct 30, 2024 · 5 comments
Open

Decouple url_base and view_name #67

SamuelJennings opened this issue Oct 30, 2024 · 5 comments

Comments

@SamuelJennings
Copy link
Contributor

At the moment, CRUDView.url_base is strongly coupled with the path name for a given role (see Role.get_url) which I don't think makes a lot of sense.

IMO, the base component of the path name should always default to cls.model._meta.model_name and MAYBE have a separate option at the class level to change it, e.g. CRUDView.path_name_base or whatever.

On another note, why provide the url_base option at all? This functionality can already be handled in urls.py using the include function.

urlpatterns = [
    path("my-url-base/", include(MyCRUDView.get_urls()),
] 

IMO (again), the above is a much simpler and easy-to-understand way to declare this as opposed to a class-based option.

@carltongibson
Copy link
Owner

Hi 👋

I'm not sure what you're proposing? (Likely answer is no 🙂)

IMO, the base component of the path name should always default to cls.model._meta.model_name

It does. That's the default value for CRUDView.url_base.

@SamuelJennings
Copy link
Contributor Author

What I'm trying to say is I don't think url_base should affect the path name at all because they are not the same thing. It leads to a loss of flexibility and potential complications down the road. For example, the following class,

class DatasetCRUDView(CRUDView):
    model = Dataset
    url_base = "my-base-url-to-show-datasets"  # a little absurd, I know :)

produces urls that look like this:

my-base-url-to-show-datasets/
my-base-url-to-show-datasets/new/
my-base-url-to-show-datasets/<pk>/
my-base-url-to-show-datasets/<pk>/edit/
my-base-url-to-show-datasets/<pk>/delete/

Great, that's exactly what I expect. However, if I want to reverse these urls in python code or templates outside of my CRUDView, I MUST refer to them by the url_base that was declared on the class.

url = reverse("my-base-url-to-show-datasets-list")

I think this gets a bit unwieldy over time and could confuse the hell out of new developers to a codebase. If the path name was not coupled so strongly to url_base then we could continue referring to this view by it's default, e.g. "dataset-list". Or, if there was a separate path_name_base, we could set it independently.

Hopefully, that's more clear 🤞

@carltongibson
Copy link
Owner

OK, makes sense. It's not API that I'd add an option for explicitly, but custom
Roles are on the way, and you'd be able to do this then by overriding get_url(). I'll leave this open as a reminder to document that.

Thanks for the follow up!

@SamuelJennings
Copy link
Contributor Author

Custom Roles sounds like a great feature 👍 I'll keep an eye out for it.

Thanks for the package btw! Helped me streamline a lot of code from multiple projects.

@carltongibson
Copy link
Owner

Glad you're enjoying it, and thanks again for the feedback 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants