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

[Feature] Get Papers #8

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

[Feature] Get Papers #8

wants to merge 23 commits into from

Conversation

TomasSzeSirk
Copy link

@TomasSzeSirk TomasSzeSirk commented Sep 3, 2022

Motivación

Hacemos este endpoint para que el frontend del bot de discord pueda obtener los links de los papers del cuatrimestre.

Endpoints

En el futuro se agregará la documentación pertinente en el README

GET /api/discord/v1/papers - Obtiene los links y los nombres de los papers

Prerrequisitos

Debes tener Rust instalado. Para instalarlo ejecutá los siguientes comandos.

curl https://sh.rustup.rs -sSf | sh -s -- -y

source "$HOME/.cargo/env"

Build and Run

Ejecutar el siguiente comando compilará y ejecutará el binario que estará escuchando requests en el puerto 8080 en localhost por default.

make run

La directiva de arriba admite las siguientes opciones:

  • DOMAIN: el dominio en el que se alojará el backend. Es http://127.0.0.1 por defecto (en un futuro https).
  • PORT: el puerto en el que el backend escuchará requests. Es 8080 por defecto.

Para probar

El Makefile dispone de la siguiente directiva para probar el endpoint:

make test_get_papers

ilitteri and others added 22 commits August 20, 2022 19:27
Tests don't run because of a tokio runtime bug
This way will allow extensibility for other API needs
Add prerrequisites section
This way will allow extensibility for other API needs
Add how to run tests section
Rename section
* Add test coverage workflow

* Downgrade codecov version

* Add rusty-hook

* Remove codecov for the moment because of a bug

* Add CircleCI for fmt, clippy and test running

* Fix rust version in workflow

* Fix rust version in workflow for allthe jobs

* Remove .tarpaulin for the moment
@TomasSzeSirk TomasSzeSirk self-assigned this Sep 3, 2022
@TomasSzeSirk TomasSzeSirk changed the base branch from main to help_queue_mvp September 3, 2022 19:08
@TomasSzeSirk TomasSzeSirk marked this pull request as ready for review September 3, 2022 19:21
Copy link
Member

@ilitteri ilitteri left a comment

Choose a reason for hiding this comment

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

El código funciona y está muy bien el PR en general. Marqué algunos detalles para refactorizar nada más. En el futuro tratá de que los commits sean autocontenidos, es decir que por ejemplo, los cambios de dependencias hechos en Cargo.toml y Cargo.lock pertenezcan a un commit tipo "Agrego dependencias". Te recomiendo leer How to make your code reviewer fall in love with you y How to Write a Git Commit Message

@@ -13,6 +14,12 @@ struct Requester {
voice_channel: u64,
}

#[derive(Serialize, Deserialize)]
struct Papers {
Copy link
Member

Choose a reason for hiding this comment

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

El nombre del struct hace referncia a papers en plural pero parece que se trata de un struct para un paper individual.

Comment on lines +245 to +264
.map(|paper_url_github| {
let paper_name = paper_url_github
.split(&['/', '.'][..])
.collect::<Vec<&str>>()[9]
.to_string();

let mut paper_url_to_modify =
paper_url_github.split_inclusive('/').collect::<Vec<&str>>();
paper_url_to_modify.remove(4);
paper_url_to_modify.remove(3);
paper_url_to_modify.remove(1);
paper_url_to_modify.remove(0);
paper_url_to_modify.insert(0, "https://");
let paper_url_string = paper_url_to_modify.concat();

Papers {
name: paper_name,
url: paper_url_string,
}
})
Copy link
Member

@ilitteri ilitteri Sep 3, 2022

Choose a reason for hiding this comment

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

Creo que toda esta función puede ser un try_from del struct Paper para que intente armarlo desde el url. ¿Qué les parece @IAvecilla @josuebouchard ?

@@ -13,6 +14,12 @@ struct Requester {
voice_channel: u64,
}

#[derive(Serialize, Deserialize)]
Copy link
Member

Choose a reason for hiding this comment

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

No estoy del todo seguro pero creo que no hace falta que derive Deserialize porque en ningún momento lo deserializamos.

/// Create papers.json
async fn get_papers() -> Result<impl Reply, Rejection> {
let body = reqwest::get("https://github.com/algoritmos-iii/algoritmos-iii.github.io/tree/master/assets/bibliografia")
.await.map_err(|e| reject::custom(ServerError::Request(e.to_string())))?
Copy link
Member

Choose a reason for hiding this comment

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

Pongamos un new line después del await.

Copy link
Member

Choose a reason for hiding this comment

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

Aunque va a ser mejor que implementemos OrRejected para el Result de reqwest::get.

Copy link
Member

Choose a reason for hiding this comment

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

Pero lo último podemos dejarlo para un PR aparte.

let body = reqwest::get("https://github.com/algoritmos-iii/algoritmos-iii.github.io/tree/master/assets/bibliografia")
.await.map_err(|e| reject::custom(ServerError::Request(e.to_string())))?
.text()
.await.map_err(|e| reject::custom(ServerError::Request(e.to_string())))?;
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Base automatically changed from help_queue_mvp to main September 6, 2022 23:21
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.

2 participants