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

pathForType/buildURL should dasherize CamelCase types #19

Closed
dustinfarris opened this issue Nov 18, 2014 · 11 comments
Closed

pathForType/buildURL should dasherize CamelCase types #19

dustinfarris opened this issue Nov 18, 2014 · 11 comments
Labels

Comments

@dustinfarris
Copy link
Owner

To reproduce

  1. DRFAdapter.pathForType('FurryAnimals');

Expected result

  • furry-animals

Actual result

  • furryanimals
@dustinfarris dustinfarris added this to the Version 1.0 milestone Nov 18, 2014
@benkonrath
Copy link
Collaborator

I initially used the dasherized version but @g-cassie pointed out that DRF defaults to the lower-cased pluralized model name without dashes. There's even a note about this in the README:

https://github.com/dustinfarris/ember-django-adapter/blob/version-1.0/README.md#path-customization

Personally I'll always use the dasherized version because I think it's easier to read and it's more like django slugs but I'm also ok with overriding this to my preference. We should just pick one version and document it. I'm ok if it goes back to the dasherized version.

@dustinfarris
Copy link
Owner Author

I must have missed the discussion. I didn't realize DRF made a decision in that regard.

My understanding is that the user makes this distinction when creating a router, e.g.:

router = DefaultRouter()
router.register(r'furry-animals', viewsets.FurryAnimalViewSet)

I realize that DRF does a lowercase dash-less version of the model name when creating internal references, e.g.:

reverse('furryanimal-list')

which is more or less in line with how Django handles ContentTypes; but this has nothing to do with the URL. Or am I missing something?

@benkonrath
Copy link
Collaborator

Yeah, you're correct. I thought that the URL would be generated if you didn't provide it. I just read the DRF code and that's not correct. @g-cassie even figured out the initial mistake about confusing the URL and the internal reference in #11 but I still thought keeping the change in - I don't remember why, sorry. I agree with changing to the dasherized version.

benkonrath added a commit to benkonrath/ember-django-adapter that referenced this issue Nov 19, 2014
benkonrath added a commit to benkonrath/ember-django-adapter that referenced this issue Nov 19, 2014
@g-cassie
Copy link

My other point was that @detail_view and @list_view default to underscorized urls which is why I thought it made sense to default to underscore on the adapter side. I think either way works but you should add docs for setting up @detail_view and @list_view to have dasherized urls.

@dustinfarris
Copy link
Owner Author

related conversation regarding detail_view/list_view URLs:

encode/django-rest-framework#2010

it looks like @tomchristie is on board with at least making dashes an option for these endpoints, although it may not happen for a while.

regardless, to my knowledge there is no native Ember functionality that makes use of custom endpoints like these (correct me if I'm wrong) so our dasherizing policy should stem from best/common practices seen in basic CRUD URLs

@dustinfarris
Copy link
Owner Author

Resolved in #28

@g-cassie
Copy link

I haven't looked at the code for your rewrite in detail but I remember Toran's adapter required urls for nested objects like /dealerships/1/cars/ and I was using @detail_view to implement these. I can't remember how multi-word models were treated. In any event if you want to support this kind of route structure than you should consider the following viewset.

class ZooViewSet(viewsets.ModelViewset):

    @detail_view(methods=['get'])
    def fuzzy_animals(self, request):
         queryset = self.get_object().fuzzy_animals.all()
         serializer = FuzzyAnimalSerializer(queryset, many=True)
         return serializer.data

@dustinfarris
Copy link
Owner Author

Good point. We will need to investigate this further. I vaguely recall thinking that endpoints for nested relationships like this were no longer necessary, but I can't remember what the reasoning was, or if I was even correct.

Anyway, we should probably open a new issue for this specific requirement.

@g-cassie
Copy link

You may be thinking of the coalesceFindRequests option that was introduced in beta.9. However DRF does not natively support the requests ED generates with this option enabled so I think there is still work to be done on the adapter side.

@dustinfarris
Copy link
Owner Author

Gotcha. Would you mind opening an issue for it?

@g-cassie
Copy link

Done: #32

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

No branches or pull requests

3 participants