Skip to content

Conversation

luca-medeiros
Copy link
Contributor

  • added pre-commit configs on a followup from previous PR Add ruff linter #83
  • added related docs
  • ran the pre-commit on all files of main

@luca-medeiros
Copy link
Contributor Author

Ive just seen the works of #91. @mithun2003 mind doing some review here?

@mithun2003
Copy link

Is this pre-commit check for print statements in the code beneficial? It ensures that prints are reviewed before committing changes.

@luca-medeiros
Copy link
Contributor Author

@mithun2003 seems there are only two print statements in this repo. Not sure print reviews are needed.

@igorbenav igorbenav linked an issue Jan 2, 2024 that may be closed by this pull request
@igorbenav igorbenav added the enhancement New feature or request label Jan 2, 2024
@igorbenav
Copy link
Collaborator

Maybe we should remove the print statements at once and use proper logging for the worker, checking whether there are print statements would be useful in this case.

By the way, how do you guys feel about moving the worker script from app to core/utils?

@luca-medeiros
Copy link
Contributor Author

Agree with the proper logging. What about creating a worker folder under core?
app/worker.py -> app/core/worker/settings.py, app/core/worker/functions.py

That much depth maybe too much and redundant for now, but I believe there is room for expansion for arq worker.

@igorbenav
Copy link
Collaborator

Yeah, I think it's a good approach. Stuff like dynamic prioritization, scheduling, custom retry would be cool.

@luca-medeiros
Copy link
Contributor Author

luca-medeiros commented Jan 4, 2024

Yeah, lots can be done with workers.

@igorbenav any thoughts about the PR? I thought about creating a separate part on readme to further explain pre-commit to the people who fork the repo. For now, I only added an extra step to the Contributing part.

@igorbenav igorbenav self-assigned this Jan 4, 2024
@igorbenav
Copy link
Collaborator

I think it's a good idea, @luca-medeiros. Do you want to add it to this PR or do it in the future? I'll merge it.

@luca-medeiros
Copy link
Contributor Author

@igorbenav refactored the contributing docs.

@igorbenav igorbenav merged commit 27816d4 into benavlabs:main Jan 4, 2024
@igorbenav igorbenav mentioned this pull request Feb 7, 2024
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre Commit
3 participants