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

Data Structures enhancement with PEP8 #1

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

Conversation

jpcadena
Copy link

@jpcadena jpcadena commented May 26, 2023

Pull Request - Implementation of CSV Data Loading, Analysis Functions, and Code Quality Enhancements

Hello dear Camila,

Description of Changes

This Pull Request presents the implementation of the various functions required to load, manipulate, and analyze data from the CSV file as per the instructions in the Exercise section of the README file, with a strong focus on adhering to Python's best practices and enhancing code quality.

Specific Changes:

  1. CSV Data Loading: Implemented a function in the Student class to consistently load the CSV data from a fixed path (data/raw/) relative to the script's location. This function validates the age field to ensure that only valid student data is loaded.

  2. Data Querying and Analysis Functions: Implemented several functions to perform specific data queries and analyses as per the Exercise requirements.

  3. Report Generation: Implemented a function to write the generated reports to separate CSV files.

  4. User Interface: Created a main program that provides a user interface for interacting with the functions and generating reports based on user input.

  5. Cross-platform Compatibility: Ensured that the implemented features are compatible across Windows, MacOS, and Unix platforms.

  6. PEP8 Compliance: Ensured the code adheres strictly to PEP8 standards.

  7. Type Hints and Mypy: Applied type hints throughout the code for better readability and understanding of the expected data types. Utilized Mypy for static type checking to catch any potential errors.

  8. Pylint, Black, and GitHub Actions: Utilized Pylint and Black for maintaining the coding standard and style formatting. Implemented GitHub Actions for Pylint to automatically check code quality on each push to the repository.

  9. Pre-commit Configuration: Added pre-commit configuration to automatically run isort, Black, Pylint, and Mypy before each commit, ensuring consistent code quality.

  10. Documentation: Thoroughly documented all modules, classes, and functions using reStructuredText docstrings to explain what they do and how they work.

Impact on Exercise Tasks

The changes implemented cover all the tasks specified in the Exercise section, providing a comprehensive solution for loading, manipulating, and analyzing the student data from the CSV file. Additionally, the improvements in code quality and compliance with best practices ensure that the code is maintainable, scalable, and robust.

Request for Review

I kindly request a review of these changes. Your feedback is highly appreciated.

Thank you for your time and consideration. Looking forward to your feedback.

Best Regards,

Juan Pablo Cadena Aguilar

students: list[dict[str, Any]] = []
file_path: str = os.path.join(
os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
self.csv_file.replace("/", "\\"))
Copy link
Owner

Choose a reason for hiding this comment

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

Esta linea rompe el funcionamiento en sistema unix.

:return: An iterator over the filtered students
:rtype: Iterator[dict[str, Any]]
"""
return (student for student in students if student[key] == value)
Copy link
Owner

Choose a reason for hiding this comment

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

Esta función es muy util! la prodrias aprovechar mucho mas, no solo para escribir los reportes.

return {
group: sum(student[average_key] for student in students) / len(students)
for group, students in groups.items()
}
Copy link
Owner

Choose a reason for hiding this comment

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

Dict comprehension como este son muy complejor y poco legibles para otros desarrolladores, ya que toca hacer un esfuerzo grande para entender que se hace.

:type average_key: str
:return: A dictionary where keys are the unique values of the group
key, and values are the average of the average key for each group
:rtype: dict[Any, float]
Copy link
Owner

Choose a reason for hiding this comment

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

Podrías devolver el promedio de edad como un número entero y no como un Float? las edades raramente se evaluan con decimales.

:return: The list of unique values
:rtype: list[str]
"""
return list({item[key] for item in data if key in item})
Copy link
Owner

Choose a reason for hiding this comment

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

Esta función tambien creo que podría ser mas ultilizada en diferentes partes del código, como por ejemplo en la implementación que tienes en city_with_most_careers

README.md Outdated
2. Clone the **repository**
```
git clone https://github.com/jpcadena/dojo_datastructures.git
```
Copy link
Owner

Choose a reason for hiding this comment

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

Si se está realizando un fork, no se debería descargar este reporsitorio :)

```
pip install -r requirements.txt
```

Copy link
Owner

Choose a reason for hiding this comment

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

puedes agregar como ejecutar el proyecto?

set(student["ciudad"] for student in students)
}
max_city: str = max(
city_career_counts, key=lambda city: len(city_career_counts[city]))
Copy link
Owner

Choose a reason for hiding this comment

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

Qué pasa si existen 2 ciudades con el mismo número de estudiantes?

city: {student["carrera"] for student in students if
student["ciudad"] == city} for city in
set(student["ciudad"] for student in students)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Esta linea es poco legible, al tener 3 comprehensions en la misma expresión hace dificil seguir la funcionalidad.

Logan,above
Elijah,above
Mateo,below
Sofia,above
Copy link
Owner

Choose a reason for hiding this comment

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

Puedes agregar mas información a este reporte? piensa en si fueras alguien externo que lo va a leer, que información le darias para que entienda que se le está entregando

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