>- requirements.txt is the standard for specifying deps, right?
poetry is a new thing. I just tried it for this project it replaces requirements.txt with pyproject.toml
>- making a README explaining the project might be good
I did. It's documentation.md. I didn't title it README.md because I wanted the front page on github.com to show the takehome instructions rather then my docs on it.
>- tests?
This was addressed in the documentation.md. I was given 4 hours to work on this problem so I just used manual tests.
>- building the flask application via a function is a bit more testable
Flask is an IO app. It's inherently not testable via unit tests because it's a server. You'd have to build integration tests around an entire server which is huge overkill for this project. Testable logic is usually pure and stateless, that's located in utils.py. It's ok this is good criticism.
You can monkey patch or make your code 10x more complicated with modules that accept mockIO for dependency injection but I'm actually against this style of programming as it over complicates the code with little benefit.Trying to test Flask by mocking everything out is basically just testing the mocks. Better to do this haskell style and segregate IO from pure functions (see utils.py). IO can't be unit tested.
Not many people understand the "testing" philosophy I'm following here so this is good feedback. I should sometimes just follow what's popular but ultimately not the "best" way in my opinion.
>- - output not sanitized for /test endpoint
this I don't get? Sanitized? The http request returns typical python structs, which are automatically converted to Json by flask/quart.
>- did you have to define your own Json type? Is that complete? Where’s null?
Nope don't have to, type checking is optional in python, but why not? You have to do this to get correct type annotations to everything. You're right about this missing Null/None. It all still type checks.
>- generator in util could have been pulled out probably? It’s created every time
Why pull it out? A generator is cheaper then creating a list from a list comprehension every time. instead of storing every value in a list and iterating through it, I just iterate through a generator. Saves memory and runtime cost is still O(N) regardless.
>- - url generate function should probably template hostname — that’s more important than host, most of the time for running in different environments
Sure but the context is a takehome project and it's running in docker-compose as specified by the directions. I mean yes, I can make that utility function more general for sure.
>- trailing slashes matter in flask, evidently, and every request without one gets redirected (test suite would have caught this)
I'm aware of this, it's not a mistake. I left it in as valid. The redirect does not get counted so api/xxx/ is equivalent to api/xxx
- on the usage of redis, I wonder if scanning + in-memory aggregation is better… zincr/zrank/zrevrange is good, but you’ll have to hold all data in memory (and receive it in one large response) and logN anyway, might as well do it simply with a set with a dynamic prefix and scan while building the output data structure as you go.
Your way is NlogN sorting and constant time inserts. The current way is zero cost sorting and logN inserts.
LogN is blazing fast, while nlogn can get slow if N gets too large. See this for relative visualization: https://i.stack.imgur.com/osGBT.jpg. Given the picture I would say my way is better. .
>- - do your API endpoints return JSON?
Yes this is the default. If you return anything that's equivalent to that JSON type I defined in utils.py, flask will automatically return serialized json.
> - error handling around points of failure like redis
Flask/quart runs under the hypercorn server (see the dockerfile). Additionally If a handler throws an exception the user automatically gets a 500 error from flask it's handled exactly as you would expect an http server should handle it. The server does not crash.
>— your app goes down if the connection is flaky right?
No it does not. hypercorn remains running if the python worker crashes. But of course if hypercorn itself goes down then it's done. This of course can be mitigated by systemd but that's overkill for this project.
>- zrevrange is deprecated now btw
Yeah your right. My mistake. Still works though. So it's marked for deprecation, but not actually deprecated yet.
>-healthz endpoints?
>-metrics?
>-tracing?
>-error reporting (ex. Sentry)
For a four hour take-home project? These things weren't even in the spec and how these things and if these things are implemented are extremely variable per company across the industry.
You know overall your post has been enlightening but not in the way you think because we are clashing on most of these points. I can see how developers can literally disagree on everything and how code reviewers can make a ton of assumptions and have a ton of arbitrary opinions. Not to mention varying levels of experience and what experience for that matter causes them to make completely different judgement calls. Don't take this as a slight, I'm sure if I was in your position I would be reviewing your code in the same way and you'd see me in the same way as well.
I'm starting to think take home assignments are just bad. Even worse for evaluating programmers then algorithm interviews because there's so many biased variables here that programmers are just clashing on. Maybe it's a good filter for finding programmers who view the programming world in the exact same way they see it.