kpi-web-guild / django-girls-blog-OlenaEfymenko

django-girls-blog-OlenaEfymenko created by GitHub Classroom
MIT License
1 stars 0 forks source link

Initialize Django project #22

Closed OlenaYefymenko closed 10 months ago

OlenaYefymenko commented 12 months ago

This PR provides:

📎Initial configurations for the project; 📎Required project dependencies; 📎Pylint configuration for Django project.

Ref #18

OlenaYefymenko commented 11 months ago

@webknjaz I'm uncertain about what to do if, after running the command python -m pre_commit run --all-files, pre-commit tool automatically edited several files in my project (formatting changes). Is it correct to commit these within this PR, especially in the context of an atomic commit? I also plan to add this commit to .git-blame-ignore-revs. That part is clear to me, but I'm unsure about handling different PRs again. I understood that running the pre-commit tool should be together with the first commit Initialize Django project. But, now I have another wrong situation. I'm trying to figure out how I can resolve this, so we can finally merge this PR

webknjaz commented 11 months ago

If the formatting happened to the newly added lines, include it into those commits that introduce these lines. So the PR would have commits that add properly formatted right away and those changes won't be in separate commits. Formatting of unrelated changes is usually related to updating linter versions or adding new tools — those would need to be bundled in separate PRs.

OlenaYefymenko commented 11 months ago

If the formatting happened to the newly added lines, include it into those commits that introduce these lines. So the PR would have commits that add properly formatted right away and those changes won't be in separate commits. Formatting of unrelated changes is usually related to updating linter versions or adding new tools — those would need to be bundled in separate PRs.

Thank you, I got it. What about the pylintrc config (the config file .pylintrc doesn't exist!)? Maybe we should approve PR #28 first? I have doubts that we need this config at this stage, also the PR can't be atomic in this case.

webknjaz commented 11 months ago

That's only a problem because you originally configured the CLI argument to point to a non-existent file. The correct solution is to remove the CLI argument or to create the file. Both would need an atomic PR.

OlenaYefymenko commented 11 months ago

That's only a problem because you originally configured the CLI argument to point to a non-existent file. The correct solution is to remove the CLI argument or to create the file. Both would need an atomic PR.

Оk, but I already have another open PR #28, which also can resolve this issue.

webknjaz commented 11 months ago

That's not a fix. It has a side effect of making it not fail but I'm not convinced that all those other changes are needed. If they are, each need to be explained. Let's not make changes without understanding the implications. Removing linting or CI also makes it not go red but it doesn't automatically mean it's a good idea.

OlenaYefymenko commented 11 months ago

That's not a fix. It has a side effect of making it not fail but I'm not convinced that all those other changes are needed. If they are, each need to be explained. Let's not make changes without understanding the implications. Removing linting or CI also makes it not go red but it doesn't automatically mean it's a good idea.

Okay, I pushed new PR #28

OlenaYefymenko commented 11 months ago

Sorry, I am fixing now and will push soon

OlenaYefymenko commented 10 months ago

@webknjaz it looks like we can merge, doesn't it?🙂

webknjaz commented 10 months ago

@webknjaz it looks like we can merge, doesn't it?🙂

No, I'll wait for at least two of the earlier requests to be addressed.

OlenaYefymenko commented 10 months ago

@webknjaz it looks like we can merge, doesn't it?🙂

No, I'll wait for at least two of the earlier requests to be addressed.

I already did it if I understood the recommendation correctly

webknjaz commented 10 months ago

https://github.com/kpi-web-guild/django-girls-blog-OlenaEfymenko/pull/22/files#r1350522669 https://github.com/kpi-web-guild/django-girls-blog-OlenaEfymenko/pull/22/files#r1360742991