-
Notifications
You must be signed in to change notification settings - Fork 0
Corrección Tp individual #8
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
base: base
Are you sure you want to change the base?
Conversation
noahmasri
left a comment
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.
El trabajo se encuentra aprobado. Está muy bien documentado, entendible, con muchos tests y cubre todos los casos que pedimos. Salvo por una prueba que no puedo entender por qué te falla, está perfecto (SELECT * FROM materias WHERE NOT nombre = 'Desarrollo' AND departamento = 71 OR codigo > 75;, me trae materias como 71,75,Desarrollo, que se llama desarrollo, por lo q no cumple la primera condicion, y además su codigo es 75, por lo que tampoco cumple la segunda). Tenés tests muy abarcativos por lo que lograste que nuestras pruebas no puedan romperlo. Incluso debo destacar que es el primer trabajo que corrijo que considera las comparaciones entre dos columnas una posibilidad. Felicitaciones y suerte en lo que resta de la materia!
| >```BASH | ||
| >cargo doc --open | ||
| >``` | ||
| > Alternativamente se puede visualizar por [github!](https://gabokatta.github.io/rustic-sql) |
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.
muy lindo quedó!
| /// | ||
| /// `DeleteBuilder` se encarga de construir una consulta de eliminación a partir | ||
| /// de una lista de tokens que representan la sintaxis de una consulta SQL. | ||
| pub struct DeleteBuilder { |
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.
parece que se encarga de eliminar un builderjsjsj
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.
JAJAJA tenes razon, pasa que el IDE me chillaba si el nombre del struct era redundante
Mas claro era DeleteQueryBuilder pero como ya inferia que estaba dentro del modulo Query me molestaba con eso 🤣
| /// | ||
| /// # Errores | ||
| /// | ||
| /// Retorna un error `Errored` si ocurre algún problema durante el análisis. |
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.
que significa errored?
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.
Es una palabra que he visto mucho en el laburo y en otros lugares/tutoriales, yo la entiendo como un termino que engloba todo lo que puede ser erroneo, no estoy 100% seguro si es ingles valido pero veo que se usa bastante y queria diferenciar mi Error generico del de std rust.
| _ => unexpected_token_in_stage("COLUMN", t)?, | ||
| } | ||
| } | ||
| Ok(fields) |
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.
y si pongo los valores pero desordenados? es mejor que vaya comparando de a uno, spliteando por las keywords en el orden en que deberían aparecer
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.
Noah, me podrias dar un ejemplito de un caso que rompa esto?
No lo estaria viendo el error :(
| } | ||
| delete_temp_file(&self.table_path, &temp_path)?; | ||
| Ok(()) | ||
| } |
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.
buenísimo, no cargas todo en memoria
| Insert => executor.run_insert(), | ||
| _ => errored!(Syntax, "unknown operation trying to be executed."), | ||
| } | ||
| } |
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.
bienn
| pub conditions: ExpressionNode, | ||
| /// El criterio de ordenamiento para los resultados. | ||
| pub ordering: Vec<Ordering>, | ||
| } |
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.
quizas seria mejor separar por querys aca, que sea un enum, ya que hay campos que si son select van a estar vacios, como "updates" o "selects"
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.
muy buena observación! Lo considere en su momento pero creo que me dije a mi mismo "ehhhh capaz estoy haciendo todo muy parecido a como lo encararía en Java" (lenguaje que uso usualmente laburando) asi que descarte la idea, pero me lo llevo para el tp grupal
No description provided.