Skip to content

Conversation

@rudyryk
Copy link

@rudyryk rudyryk commented Dec 7, 2013

Hello, Charles! I'm currently using flask-pewee in production and found custom actions feature would be very useful, e.g. content moderation, users blocking etc. I've made a simple implementation, how do you like it?

Copy link
Owner

Choose a reason for hiding this comment

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

Why not (self.delete, {'url': '/delete/', 'title': 'Delete'}) ?

Copy link
Owner

Choose a reason for hiding this comment

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

Or, since the "string" name of the view is pretty important, a 3-tuple:

(self.delete, 'delete', {'url': '/delete/', 'title': 'Delete'})

@coleifer
Copy link
Owner

coleifer commented Dec 8, 2013

I've typically preferred to point directly to the function on the view class, rather than relying on a name/getattr. Aside from that it looks good. Do all tests pass, and do you think it is worthwhile to add any new tests?

@rudyryk
Copy link
Author

rudyryk commented Jan 13, 2014

Hi Charles! I agree with your considerations, I'll rewrite the code with more explicit 3-tuple logic and I think it would be right to add tests for registering actions urls.

@AlJohri
Copy link

AlJohri commented Feb 20, 2014

+1 for this PR

@rudyryk
Copy link
Author

rudyryk commented Mar 28, 2014

Hi Charles! I've updated actions definitions to point directly to methods, but I still use 2-tuple. All tests pass. Looks ok?

@coleifer
Copy link
Owner

This looks fantastic. Do you mind adding tests for the new behaviors? Perhaps a custom action and then testing to ensure it works correctly?

@rudyryk
Copy link
Author

rudyryk commented Mar 30, 2014

Well, I believe most cases are covered by existing tests, because delete and export actions are defined in the new generic way. Custom actions have no difference with export and delete actions.

But I don't mind if we add extra tests, I just need some hints how to setup test case. I took a look at existing tests and failed to find the most appropriate place to insert new one for this case :)

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

Successfully merging this pull request may close these issues.

3 participants