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

Seating charts - 2024 edition #35

Merged
merged 31 commits into from
Apr 7, 2024
Merged

Seating charts - 2024 edition #35

merged 31 commits into from
Apr 7, 2024

Conversation

carl-vbn
Copy link
Contributor

@carl-vbn carl-vbn commented Mar 3, 2024

This PR is here to make things a lot easier to work with

  • Added imagedl.py script to facilitate image downloading
  • Added roster.py to download rosters. It is also imported by other scripts to standardize roster parsing from CSV files (it can reliably load csv files with or without canvas headers)
  • go_brr now imports seatingchart instead of calling it with os.system
  • Updated README.md
  • A counter of selected seats is now shown on the HTML pages, to make headcounts easier

fixes #32 #26 and starts on #29

@carl-vbn carl-vbn requested a review from tzussman March 3, 2024 06:04
Copy link
Member

@tzussman tzussman left a comment

Choose a reason for hiding this comment

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

Overall this looks really good! I've left a bunch of small comments throughout.
My main concern at the moment is rosters.py -- I didn't take too close of a look at it, but I'm worried that it duplicates functionality that we have in get_class.py in cloud-mgt. More importantly, it looks like it uses a different API endpoint and does some more parsing than get_class does, so I just want to make sure that there aren't some small issues we're missing there (actually it looks like it does it better than get_class so there may be some improvements to make there). It could also do with some more documentation about what format it expects as input and the structure of the roster it returns in Python.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
go_brr.py Outdated Show resolved Hide resolved
go_brr.py Outdated Show resolved Hide resolved
go_brr.py Outdated Show resolved Hide resolved
rosters.py Outdated Show resolved Hide resolved
seatingchart.py Outdated Show resolved Hide resolved
seatingchart.py Outdated Show resolved Hide resolved
rosters.py Outdated Show resolved Hide resolved
rosters.py Outdated Show resolved Hide resolved
@carl-vbn
Copy link
Contributor Author

@tzussman I implemented pretty much all of your feedback.

For rosters.py, I see your point. I added that file because it is very straightforward, not that many moving parts and allows the entire repo to be self-sufficient from retrieving all the files to generating the html. The advantage of the endpoint I'm using is that it returns the entire class in one query (I double checked, the number of entries it gives matches the number of valid rows in the CSV exported from the Grades tab).

If you think we should rely on cloud-mgt instead, I can easily strip the downloading functionality out of it. The file is also used by the other scripts to parse roster files, but none of them use the downloading functionality, that's fully manual. I've also added some more details on why this file is useful vs just loading the CSV in the other files directly.

@carl-vbn carl-vbn requested a review from tzussman March 26, 2024 01:42
Copy link
Member

@tzussman tzussman left a comment

Choose a reason for hiding this comment

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

Left a couple more small comments – this is almost there!
Re: rosters.py, I think it's fine to use it here. In fact, I think you should look into changing the cloud-mgt version to use the endpoint you found, since it seems much simpler.

imagedl.py Outdated Show resolved Hide resolved
imagedl.py Show resolved Hide resolved
rosters.py Show resolved Hide resolved
rosters.py Outdated Show resolved Hide resolved
rosters.py Show resolved Hide resolved
rosters.py Outdated Show resolved Hide resolved
@carl-vbn
Copy link
Contributor Author

carl-vbn commented Apr 4, 2024

@tzussman I hope this covers it! I also made rosters.py print things out a bit more nicely using a table, because it was just printing a big blob of unis before.

@carl-vbn carl-vbn requested a review from tzussman April 4, 2024 23:16
Copy link
Member

@tzussman tzussman left a comment

Choose a reason for hiding this comment

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

This looks great, thanks Carl! Assuming you tested the latest round of updates, feel free to go ahead and merge! (I didn't closely read the print_table() code, but I trust that it looks a lot better now)

@tzussman tzussman linked an issue Apr 7, 2024 that may be closed by this pull request
@carl-vbn
Copy link
Contributor Author

carl-vbn commented Apr 7, 2024

I tested it all with the last changes before committing, so it should be good to go then. Thanks for all your feedback Tal!

@carl-vbn carl-vbn merged commit 168aed4 into master Apr 7, 2024
@tzussman tzussman deleted the carls-rework branch April 10, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up README and documentation Get rid of the os.system call in go_brr
3 participants