-
Notifications
You must be signed in to change notification settings - Fork 3
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
Avances API Rest, cambios schema y scrapers #19
Conversation
- The plural of campus is campuses [The Britannica Dictonary]
Places can easily grow from 50, the default pagination.
Ahí surgiría el problema de como se obtiene cuál es el requisito más actualizado. Creo que lo ideal sería configurar SQLAlchemy / SQLModel para que obtenga esa lista por defecto, así se deja el mismo funcionamiento que hay en plataformas como Planner. |
Ya está resuelto el tema de la estructura de los prerequisitos. Probé los scrapers y pude construir la base de datos con todo el catálogo y lo que está público del buscacursos (tengo el backup, si alguien lo quiere, pida). Diría que conviene hacer el PR antes de seguir alargando la rama. |
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.
En general se ven muy buenos los cambios 🚀
Creo que se puede mejorar un poco la legibilidad del código, evitando cosas como mucha anidación o funciones "mágicas", lo que se arreglaría fácilmente (creo) con funciones auxiliares y docstrings.
No he probado el código aún, y en unos días no voy a poder seguir revisando hasta luego de unas 3~4 semanas, así que fuera de esta revisión no creo que pueda revisar o aportar más en estas vacaciones 😬
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!
This pull request introduces 2 alerts and fixes 6 when merging ec4e2bc into 05dc492 - view on LGTM.com new alerts:
fixed alerts:
|
@nico-mac Resolví todos los conflictos en favor de tu PR, no eh logrado correr el proyecto así que si puedes ver que no se rompiera nada estaría OK! También eliminé las rutas de facultad pues el modelo no existía. @benjavicente que falta para dar aprobada esta PR? Porque si funciona la PR es mejor pasarla a |
Creo que lo importante es que queden algunas funcionalidades de esta PR más documentadas u ordenadas (principalmente lo de Pero como dices podríamos unirla y resolverlos aparte. |
This pull request introduces 2 alerts and fixes 1 when merging 044d917 into b5667ca - view on LGTM.com new alerts:
fixed alerts:
|
This pull request fixes 1 alert when merging 1e02813 into e4b6041 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 6e2947b into e4b6041 - view on LGTM.com fixed alerts:
|
Hice issues para todos los cambios pendientes y dejé varios comentarios entre la magia de la conversión de requisitos a DNF. Corregí también un par de alertas del bot lgtm. |
API Rest:
Courses
/campuses/places
/courses
para búsqueda de cursos./courses/:id
/courses/:id/banner
para cupos (pendiente)/courses/:id
/places
/schools/:id/subjects
/subjects/:code
/courses/:code/sections
Schema:
need_all_requirements
.is_active
yequivalencies_raw
(estas también pueden ser grupos And y Or en realidad)Scrapers:
Que falta:
/courses