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

Remove duplicate code #80

Open
dgarnitz opened this issue Oct 27, 2023 · 3 comments
Open

Remove duplicate code #80

dgarnitz opened this issue Oct 27, 2023 · 3 comments
Labels
good first issue Good for newcomers tech-debt Technical Debt. Old bad code we need to fix

Comments

@dgarnitz
Copy link
Owner

Methods such as update_batch_and_job_status are duplicated across the code to prevent different modules from being dependent on each other.

We need to 1) find and document all the repetitive code 2) move it into a shared or utils location 3) make sure the duplication is removed from the existing code

@dgarnitz dgarnitz added good first issue Good for newcomers tech-debt Technical Debt. Old bad code we need to fix labels Oct 27, 2023
@Aleksandir
Copy link

Aleksandir commented Oct 27, 2023

Hi Dgarnitz,

I just wanted to confirm before i get ahead of my self, the update_batch_and_job_status is located in app.py, worker.py, and vbd_upload_worker.py

Is the goal here to move these duplicate functions to the shared/utils.py file, and import the necessary dependencies?

I have actioned according to the above in PR #83

Cheers,
Aleks

@dgarnitz
Copy link
Owner Author

Hey thanks for your interest in this task. Yes, the goal is to remove that duplicate method from those locations and move it into the shared location. shared/utils.py is good for now. We can refactor that file once it grows a bit more.

As of today, we merged a new big PR and there is now an extractor.py where this method also lives.

@dgarnitz
Copy link
Owner Author

@Aleksandir I see you opened a PR for it - great work! Really appreciate it!!

Make sure you rebase the new code that just merged. Its quite a big merge, you will need to tear down the docker contains, and re-run all the docker compose stuff. Also make sure to pull the minio docker image.

Once you do that, please retest to confirm stuff still works and tag me on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers tech-debt Technical Debt. Old bad code we need to fix
Projects
None yet
Development

No branches or pull requests

2 participants