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

Replace print with logging in application.py #1612

Merged
merged 11 commits into from
Mar 8, 2023

Conversation

diivi
Copy link
Contributor

@diivi diivi commented Feb 26, 2023

#1611
Added flake8-print,
referred the flake-8 docs for additional-dependencies - https://flake8.pycqa.org/en/latest/user/using-hooks.html

@real-yfprojects real-yfprojects linked an issue Feb 26, 2023 that may be closed by this pull request
@real-yfprojects
Copy link
Collaborator

We do use print at some places. So try restricting the plugin to production code and add ignore comments to the valid lines.

@diivi
Copy link
Contributor Author

diivi commented Feb 26, 2023

Okay, so should I ignore the ./package directory and add ignore comments to the print statements used in ./src?

src/vorta/application.py Outdated Show resolved Hide resolved
@diivi diivi requested a review from m3nu February 27, 2023 19:59
@m3nu
Copy link
Contributor

m3nu commented Feb 27, 2023

It's good to clean up those print statements and ensure none are left after debugging. 👍

Just noting that the flake8-print package isn't very popular and looks like the thing that may become unmaintained eventually. There's also supply chain attacks from such less popular packages. So always good to think twice before adding a new dependency. Maybe one of the larger linters adds this feature by then. 🤞

m3nu
m3nu previously approved these changes Feb 27, 2023
@Hofer-Julian
Copy link
Collaborator

It's good to clean up those print statements and ensure none are left after debugging. 👍

Just noting that the flake8-print package isn't very popular and looks like the thing that may become unmaintained eventually. There's also supply chain attacks from such less popular packages. So always good to think twice before adding a new dependency. Maybe one of the larger linters adds this feature by then. 🤞

Agreed, I like ruff as a flake8 and isort alternative.
it also implements this rule: https://beta.ruff.rs/docs/rules/#flake8-print-t20

@real-yfprojects
Copy link
Collaborator

Ruff seems to gain popularity already. I don't know how stable it is at the moment but I am open for switching once it is.

@Hofer-Julian
Copy link
Collaborator

Ruff seems to gain popularity already. I don't know how stable it is at the moment but I am open for switching once it is.

Fair enough.
Regarding this PR, how about we merge the changes, but don't add the flake8-dependency?

@m3nu
Copy link
Contributor

m3nu commented Feb 28, 2023

Regarding this PR, how about we merge the changes, but don't add the flake8-dependency?

If @diivi wants to get this merged, this would be the fastest way.

For extra points, he could add Ruff and make sure it detects the print statement. And we run it in parallel with flake8 for a while. Then remove it, once we are comfortable with it. Better to add a dependency with 10k stars than 100.

@diivi
Copy link
Contributor Author

diivi commented Feb 28, 2023

Yup, ruff looks like a better alternative, I'll remove flake8-print from this PR and make a new one to add it in ruff.
Should I rename this to chore: replaced print with logging in application.py?

@Hofer-Julian
Copy link
Collaborator

For extra points, he could add Ruff and make sure it detects the print statement. And we run it in parallel with flake8 for a while. Then remove it, once we are comfortable with it. Better to add a dependency with 10k stars than 100.

Sounds good to me.

Should I rename this to chore: replaced print with logging in application.py?

If I look at past commits, Replace print with logging in application.py without the chore would be fine

@diivi diivi changed the title ci: add flake8-print linter Replace print with logging in application.py Feb 28, 2023
@diivi diivi requested a review from Hofer-Julian February 28, 2023 23:18
Hofer-Julian
Hofer-Julian previously approved these changes Mar 1, 2023
@real-yfprojects
Copy link
Collaborator

If I look at past commits, Replace print with logging in application.py without the chore would be fine

Yeah, we will squash any ways.

Copy link
Collaborator

@real-yfprojects real-yfprojects left a comment

Choose a reason for hiding this comment

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

There still is one print in production code. But I don't think replacing it would make sense.

print(f"Vorta {__version__}")

m3nu
m3nu previously approved these changes Mar 6, 2023
Copy link
Contributor

@m3nu m3nu left a comment

Choose a reason for hiding this comment

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

Looks like a big improvement. 😄

@real-yfprojects real-yfprojects dismissed stale reviews from m3nu and Hofer-Julian via 4fe0149 March 8, 2023 16:31
@real-yfprojects real-yfprojects merged commit b01fa10 into borgbase:master Mar 8, 2023
@diivi diivi mentioned this pull request Mar 21, 2023
9 tasks
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.

4 participants