New feature/context page #7
Conversation
EvanHughes-dev
temporarily deployed
to
github-pages
EvanHughes-dev
temporarily deployed
to
github-pages
EvanHughes-dev
temporarily deployed
to
github-pages
EvanHughes-dev
temporarily deployed
to
github-pages
There was a problem hiding this comment.
Nice work on this. That's one hell of a PR right there. I made one good pass trying to get an idea of what is going on with the project overall. I skimmed over some stuff and tried not to nitpick too much because it's a prototype, but I mostly wanted to make sure things are relatively clean.
Before merging, be sure to have someone download and set up everything on their computer from scratch. Also, spend some extra time trying to break your context upload stuff. While it's just a prototype, we are going to let MECEs touch it, so if there's a way to break it, they'll find it
|
|
||
| return jsonify(configData).get_data(as_text=True), 200 | ||
|
|
||
| def post(self): |
There was a problem hiding this comment.
Should Context have a member function post, or should there be a function in another class that accepts an instance of Context to post? I don't think we need to do it the right way for the prototype, but I'd be interested to see if there was a good standard
There was a problem hiding this comment.
I'm probably fine leaving it like this, but I would like some explanation for why you think it post should be a member function. To me, it seems like it would make more sense to have all the SQL in a single file instead of sprinkled across multiple, but I haven't thought through the whole design. Why do you want it this way? Or are you planning to change it later?
Co-authored-by: Matthew Magee <[email protected]>
EvanHughes-dev
temporarily deployed
to
github-pages
Co-authored-by: Matthew Magee <[email protected]>
Co-authored-by: Matthew Magee <[email protected]>
EvanHughes-dev
temporarily deployed
to
github-pages
EvanHughes-dev
temporarily deployed
to
github-pages
EvanHughes-dev
temporarily deployed
to
github-pages
EvanHughes-dev
temporarily deployed
to
github-pages
| * Call to the python API. For each of the config | ||
| * forms, get any saved configs that are in the DB. | ||
| * Set them to the corresponding dropdown option |
There was a problem hiding this comment.
Not a huge deal, but your line limit for comments is inconsistent. We should look at getting an auto-formatter after the prototype is good to go
|
|
||
| return jsonify(configData).get_data(as_text=True), 200 | ||
|
|
||
| def post(self): |
There was a problem hiding this comment.
I'm probably fine leaving it like this, but I would like some explanation for why you think it post should be a member function. To me, it seems like it would make more sense to have all the SQL in a single file instead of sprinkled across multiple, but I haven't thought through the whole design. Why do you want it this way? Or are you planning to change it later?
TelemetrySite/server/data_upload.py
Outdated
There was a problem hiding this comment.
Did you use an autoformatter to do this?
| host = os.getenv("HOST"), | ||
| port = os.getenv("PORT") | ||
| ) | ||
| database="bert", |
There was a problem hiding this comment.
@tmb7852 At some point, it would be pretty cool if the database was not named bert
EvanHughes-dev
temporarily deployed
to
github-pages
Test the crap out of. Ignore the test data. That should be cleared soon.