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

Change the the entity_type kwarg to table_name #111

Closed
joegasewicz opened this issue Dec 5, 2019 · 10 comments
Closed

Change the the entity_type kwarg to table_name #111

joegasewicz opened this issue Dec 5, 2019 · 10 comments
Assignees
Labels
discussion enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@joegasewicz
Copy link
Owner

joegasewicz commented Dec 5, 2019

Our API has 2 public methods, one of which is register_entity. This currently requires a kwarg of entity_type to donate the table name, which is confusing to the user.

Change register_entity method's kwarg of entity_type to table_name. This may result in updating other parts of the codebase, docs, README, blog and tests.

@joegasewicz joegasewicz added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed discussion labels Dec 5, 2019
@kousikmitra
Copy link
Contributor

Or should we change it to entity_name since it's more relevant to the context. We are also using register_entity as function name.

@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 5, 2019

Yea that’s a very valid observation. But for the work involved I’m not sure it’s worth the effort at the moment, to change just a prefix of an existing kwarg.

I have had some complaints from devs looking at this library and not understanding what “entity” represents... Maybe we should change the method name and therefore also change the Kwarg to table_name. I'm wondering if this would make more sense?

edit: Current situation: we derive the entity_type from the database table name.

Sent with GitHawk

@kousikmitra
Copy link
Contributor

kousikmitra commented Dec 5, 2019

yes at first I also couldn't understood what entity represents here. But since we are using Flask models then isn't it better if we change the names to model_name and register_model?

@joegasewicz after your edit to the last comment.
Edit: Okay since we derive the entity_type from the database table name then naming it to model_name will create confusion.

@joegasewicz
Copy link
Owner Author

Ok, thats much better. Let's go with model_name as the kwarg and register_model as the method name.

I also need to update the other public method name to update_token - ( i don't think we should call it update_model because it's not updating the model).

@joegasewicz
Copy link
Owner Author

@kousikmitra Let's go with table_name as the kwarg and register_model as the method name.

@kousikmitra
Copy link
Contributor

@joegasewicz can we derive the table name from the model name? Then the user can just pass the model_name to the function and we can get the table_name from there. Since when people use Models they don't give much attention to table_name(own perspective)

@joegasewicz
Copy link
Owner Author

The problem is, is that we have to handle multiple models , so we have to know which model the user wants to append to Flask's global context.

app.config["ENTITY_MODELS"] = [ UserModel, TeacherModel, ...etc ]

@kousikmitra
Copy link
Contributor

The problem is, is that we have to handle multiple models , so we have to know which model the user wants to append to Flask's global context.

app.config["ENTITY_MODELS"] = [ UserModel, TeacherModel, ...etc ]

Okay I will look into the code and will revert back to you. or we will go with the table_name approach for now.

@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 5, 2019

@kousikmitra Im going to close this issue and open a few new ones tomorrow to carry out this work, I have to go out now to play some music but will be back in the morning UK time.
Feel free to open an issue and start working on it, if we can split this work up as much as possible as it's non trivial.
thanks
Joe

@joegasewicz joegasewicz reopened this Dec 6, 2019
@joegasewicz joegasewicz changed the title Change the the entity_type kwarg to tablename Change the the entity_type kwarg to table_name Dec 6, 2019
@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 6, 2019

@kousikmitra I've reopened & updated this issue if you still want to work on it.
Im going to create another issue to change the method name.

kousikmitra added a commit to kousikmitra/Flask-JWT-Router that referenced this issue Dec 6, 2019
joegasewicz pushed a commit that referenced this issue Dec 6, 2019
Changed the the `entity_type` kwarg to `table_name` #111 and `register_entity` now throws exception if user hasn't passed `table_name` #91
joegasewicz added a commit that referenced this issue Dec 10, 2019
**Release 0.0.25** - 2019-12-10

- Replaced the the `entity_type` kwarg to `table_name` in the public method `register_entity` [Issue #111](#111)
- Renamed the `update_entity` public method to be called `update_token` [Issue #114](#114)
- Renamed the `register_entity` public method to be called `create_token` [Issue #113](#113)
- Add Models to JWTRoutes class & init_app method [Issue #119](#119)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants