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

remove 's' from entity on g context #90

Closed
joegasewicz opened this issue Nov 26, 2019 · 15 comments
Closed

remove 's' from entity on g context #90

joegasewicz opened this issue Nov 26, 2019 · 15 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@joegasewicz
Copy link
Owner

when the entity data is assigned to Flask global context it can derive from a plural table entity name (especially if the user uses the tablename attribute). So removing the 's' makes it clearer when returning the global context entity value

user = g.user 3 . # not g.users 
@joegasewicz joegasewicz added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Nov 26, 2019
@kousikmitra
Copy link
Contributor

hey @joegasewicz is this issue still open?

@joegasewicz
Copy link
Owner Author

Hi @kousikmitra yes go ahead, thank you 👌

Sent with GitHawk

@kousikmitra
Copy link
Contributor

@joegasewicz can you give a brief? I couldn't find anything like that in the source code.

@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 4, 2019

@kousikmitra Sure thing. Currently, we set our entity object onto Flask's global context:
https://github.com/joegasewicz/Flask-JWT-Router/blob/3e3b04f93d44ed0bd6bedad3bca67aa5e0217521/flask_jwt_router/_routing.py#L185
So now we have access to the entity via the database table name:
users for example.
This issue is about check if the last letter is an s & if it is then stripping off that s so then we would be able to access our entity off of Flask's global context like this:
g.user
The reason is that this library is only working with a single entity, so it might be confusing to keep assigning a plural named attribute to g

thanks
Joe

@joegasewicz
Copy link
Owner Author

@kousikmitra Sorry, also since yoour last PR, i added linting and also testing on python 3.6, 3.7 & 3.8.
You can now run tox & as long as you have those 3 versions of python installed then it will give you tests & linting .

Of course, you can still run pytest with pip of pipenv and then push your code up & then tox will get run on the repo. Also , if you do it like this you will probably need top run pylint as well, although like i said this does get run with tox on the repo.
thanks
Joe

@kousikmitra
Copy link
Contributor

But @joegasewicz what if any table has s not as in plural but as normal English word? Should we just remove the s in that case too?

@joegasewicz
Copy link
Owner Author

@kousikmitra good point but its very unlikely because the resource name should be a noun & there's only 2 nouns with 's'

  • bus
  • lens

http://www.focus.olsztyn.pl/en-grammar-nouns-uncountable-s.html
if users of this library do want to use these nouns, im sure we can open an issue to add some kwargs to the main API methods we offer (update_entity & register_entity etc)

thank you
Joe

@kousikmitra
Copy link
Contributor

Okay it looks alright. I didn't knew this fact. 😸

@kousikmitra
Copy link
Contributor

@joegasewicz there's another issue. There is test function in which it has a keyword test_entities in this case the name ends with s but just removing the s doesn't sounds good. So in this type of nouns there will be the same problem.

@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 5, 2019

@kousikmitra good spot.
We can check that the last 3 letter are not 'ies'

These are the places to update:

https://github.com/joegasewicz/Flask-JWT-Router/blob/3e3b04f93d44ed0bd6bedad3bca67aa5e0217521/tests/test_routing.py#L70

https://github.com/joegasewicz/Flask-JWT-Router/blob/3e3b04f93d44ed0bd6bedad3bca67aa5e0217521/tests/test_routing.py#L77

https://github.com/joegasewicz/Flask-JWT-Router/blob/3e3b04f93d44ed0bd6bedad3bca67aa5e0217521/tests/fixtures/model_fixtures.py#L23

https://github.com/joegasewicz/Flask-JWT-Router/blob/3e3b04f93d44ed0bd6bedad3bca67aa5e0217521/tests/fixtures/model_fixtures.py#L39

https://github.com/joegasewicz/Flask-JWT-Router/blob/3e3b04f93d44ed0bd6bedad3bca67aa5e0217521/tests/fixtures/token_fixture.py#L8

The tests should now be able to take a noun such as users as a table name and then we should assert we have g.user. If there is no s or its ending in 'ies' in the table name then we dont need to update it.

Pick any name you like to replace test_entities for example users teachers admins .
We should also have 1+ test(s) with a word ending in 'ies'.

@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 5, 2019

@kousikmitra sorry, i edited the above comment to reconsider your point about words ending in 'ies'.

If you are still unsure about this update we can keep the issue open & if anyone else in future agrees with this feature than we can revisit this at a later date...

@kousikmitra
Copy link
Contributor

@joegasewicz ya this sounds good. Cause I am really not feeling this will be a good way of doing this.

@joegasewicz
Copy link
Owner Author

@kousikmitra perfectly understand, thanks for looking into this for now.
this is the code i currently use to work around this:

        if "teachers" not in g:
            return abort(401)
        teacher = g.teachers

@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 5, 2019

Maybe we should change the public API to make it more obvious that the entity_type is derived from the table name and not use entity_type
#111

@joegasewicz
Copy link
Owner Author

joegasewicz commented Dec 9, 2019

Closing this, as we now have renamed entity_key to table_name it makes this issue redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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