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

moving risk_data function to a new file and tests for Escribir_Datos_Osiris #8

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

natilou
Copy link
Contributor

@natilou natilou commented Apr 8, 2023

  • moving risk_data function and its helper function to a new file
  • moving Escribir_Datos_Osiris to a new file to avoid circular import between risk_data.py and subida.py
  • renamed the test file created in previous PR from test_get_phone_numbers.py to test_risk_data.py
  • adding tests for Escribir_Datos_Osiris()
  • replacing time for datetime library, because when I use the freezegun to test the time the file is created, there was an error with this library using time (issue from freezgun repo).
  • adding an environment variable in Dockerfile: file_directory="Subida Osiris" , if the script doesn't find this variable, uses the default value "Subida Osiris" --> this is archive with: os.getenv("file_directory", "Subida Osiris")
  • moving ipdb, pytest dependencies in Pipfile to [dev-packages] and adding freezegun to it

Documentation:

freezegun --> FreezeGun is a library that allows your Python tests to travel through time by mocking the datetime module.
tempfile.TemporaryDirectory --> This class securely creates a temporary directory. I used it to create a folder and test Escribir_Datos_Osiris, otherwise raises an Exception if there is no folder created.

…ris to a new file to avoid circular import between risk_data file and subida.py
@natilou natilou changed the title moving risk_data function to a new file moving risk_data function to a new file and tests for Escribir_Datos_Osiris Apr 11, 2023
Copy link
Member

@matiasbertani matiasbertani left a comment

Choose a reason for hiding this comment

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

LGTM. I didn't check carefully that the migrated risk methods were exactly the same but I trust that you only copy-pasted them

@matiasbertani
Copy link
Member

Nice PR. I'm thinking out loud. And we could add some things that I see here to the backlog as new tasks ( I didn't comment to make the changes here because these were badly coded by me)

  • change the name of the directory "Subida Osiris"
  • Code the method that creates the directory when is installed the app.
  • rename the function that writes the data that is going to be uploaded to Osiris

Again, I don't want you to change anything, I only point these things to be documented to add the task

@natilou
Copy link
Contributor Author

natilou commented Apr 14, 2023

LGTM. I didn't check carefully that the migrated risk methods were exactly the same but I trust that you only copy-pasted them

Yes, I only copy and paste the code in other file. I didn't change any logic. Maybe I renamed some variables to english, but not the logic

@natilou natilou merged commit 66989d5 into main Apr 14, 2023
@natilou natilou deleted the refactor-datos-riesgos branch April 14, 2023 22:51
matiasbertani added a commit that referenced this pull request Apr 15, 2023
changing the import to from, make error running the app
because date was used from the namespace datetime, y change the import
and the annotation werent't wrapped with simple commas `'` so python interpretates
this as we are trying to index a list class objetct
matiasbertani added a commit that referenced this pull request Apr 15, 2023
Fix: datetime.date and annotation from PR #8
@natilou natilou self-assigned this Apr 17, 2023
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