Skip to content
This repository has been archived by the owner on May 31, 2023. It is now read-only.

fix from_json, extract routes to files #42

Merged
merged 1 commit into from
Feb 13, 2019
Merged

fix from_json, extract routes to files #42

merged 1 commit into from
Feb 13, 2019

Conversation

jstriebel
Copy link
Contributor

This PR mainly extracts the routes into files, as the __main__.py grew too big, more routes are coming in the next PR. @fm3 What do you think about the file-structure?

Also, I noted a bug from the last PR, which I fixed here as well, see my comment at the code.

@jstriebel jstriebel self-assigned this Feb 13, 2019
@jstriebel jstriebel requested a review from fm3 February 13, 2019 09:21
black = "==18.9b0"
flake8 = "*"
isort = "*"
mypy = "*"
mypy = "==0.660"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a bug in mypy version 0.670, which was just released:
python/mypy#6364

Copy link
Member

Choose a reason for hiding this comment

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

good catch!

@@ -12,7 +12,7 @@

@dataclass(frozen=True)
class Scale:
chunk_sizes: Tuple[Vec3D]
chunk_sizes: Tuple[Vec3D, ...]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tuple type definitions are expected to specify all fields, or use the ellipsis, which is similar to lists then. This is needed to make from_json work for tuples of unknown length.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we want tuples of unknown length instead of plain lists? Does that help with hashability/typechecking etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, tuples can be cached, lists not.

@jstriebel
Copy link
Contributor Author

Also, I removed the add_dataset endpoint /api/<backend_name>/<organization_name>/<dataset_name>, which will be replaced by some logic on the upload route (POST on /data/datasets).

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Route categories seem fine. However, I think I still prefer Play’s routes list files as they have all entry points in one overview, but I suppose that’s not so easy to recreate here. It’s also not a super strong preference.

black = "==18.9b0"
flake8 = "*"
isort = "*"
mypy = "*"
mypy = "==0.660"
Copy link
Member

Choose a reason for hiding this comment

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

good catch!

@@ -12,7 +12,7 @@

@dataclass(frozen=True)
class Scale:
chunk_sizes: Tuple[Vec3D]
chunk_sizes: Tuple[Vec3D, ...]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want tuples of unknown length instead of plain lists? Does that help with hashability/typechecking etc?

@jstriebel
Copy link
Contributor Author

@fm3 Actually having something similar like the routes-files should be easy:

from .my_route import my_route
app.route("/bla/<param1>/<param2>", methods=["POST"])(my_route)

The problem here is that it's harder to see if the method's and the url's parameters match. That's why I would leave it like this for now.

@fm3
Copy link
Member

fm3 commented Feb 13, 2019

True, fine by me to leave it :)

@jstriebel jstriebel merged commit 5ea9744 into master Feb 13, 2019
@jstriebel jstriebel deleted the route-files branch February 13, 2019 11:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants