-
Notifications
You must be signed in to change notification settings - Fork 34
Merge dev into main #418
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
Merge dev into main #418
Conversation
* migration, url fixes * chom
* chom * chom
* Revert "packet glue (#407)" This reverts commit 2b4bc44. * Revert "chom (#406)" This reverts commit 8a6b501. * Revert "chom (#405)" This reverts commit a904044. * Revert "packet glue (#404)" This reverts commit ee6fc41. * Revert "migration, url fixes (#403)" This reverts commit 736d203. * Revert "Merge pull request #402 from Qelxiros/packet-glue" This reverts commit 0fc55d5, reversing changes made to 4fe950f. * Revert "glue (#401)" This reverts commit 4fe950f. * Revert "packet glue but again (navbar yippee) (#400)" This reverts commit 49c7bab. * Revert "packet glue but even more (#399)" This reverts commit 55ea7f8. * Revert "Merge pull request #398 from Qelxiros/packet-glue" This reverts commit 416bc10, reversing changes made to baf42df. * Revert "packet glue but again (#397)" This reverts commit baf42df. * Revert "Merge pull request #396 from Qelxiros/packet-glue" This reverts commit f936afd, reversing changes made to d3a0280.
add css animation to dropdown arrows
conditional/util/member.py
Outdated
| "h_meetings": h_meetings, | ||
| "c_meetings": d_meetings, | ||
| "t_seminars": t_seminars, | ||
| }, 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the response code to the actual route? This seems like it could maybe be a useful function to have independent of the actual route logic and to me it makes more sense at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually so real 👍 Big agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good I messed up my one comment but little nitpick stuff
conditional/__init__.py
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably should get rid of the whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, agree with @BigSpaceships's proposed changes
didn't look at dependency stuff
| @@ -1,5 +1,5 @@ | |||
| FROM docker.io/python:3.8-buster | |||
| MAINTAINER Devin Matte <matted@csh.rit.edu> | |||
| FROM docker.io/python:3.12-bookworm | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.12-bookworm is a pretty big image has has a lot of vulns, could a slim image be used here(10x smaller)?
https://hub.docker.com/_/python/tags?name=3.12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is a good idea, but I feel like that change is out of scope of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 definitely out of scope
| import io | ||
| from datetime import datetime | ||
| from distutils.util import strtobool # pylint: disable=no-name-in-module,import-error | ||
| from distutils.util import strtobool # pylint: disable=no-name-in-module,import-error,deprecated-module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crazy work 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly dont remember why I did this 😭 Im pretty sure I had an actual reason, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the cursedness of whatever evals csv import stuff is going on
|
Is #419 the only thing blocking this? |
move status for gatekeep response to route + fix 500 if user does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now (we do need to fix the merge conflict?)
No description provided.