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

Dev Docs: Style Guide #308

Merged
merged 2 commits into from Apr 4, 2024
Merged

Dev Docs: Style Guide #308

merged 2 commits into from Apr 4, 2024

Conversation

voetberg
Copy link
Contributor

@voetberg voetberg commented Apr 2, 2024

Additions:

  • General style rules (writing out what it's included in the flake8 precommit)
  • Rules on imports (isort rules)
  • Few words on testing (preference for using fixtures, not writing dirty tests)
  • Rough SQLAlchemy style guide. Based mostly on what I've observed, so there may be some mistakes. None of this is written down before so ?? It's rough.
  • What's included in the precommit and the github action checks

docs/developer/style_guide.md Outdated Show resolved Hide resolved
docs/developer/style_guide.md Outdated Show resolved Hide resolved
docs/developer/style_guide.md Outdated Show resolved Hide resolved
docs/developer/style_guide.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rdimaio rdimaio left a comment

Choose a reason for hiding this comment

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

Some minor comments - btw, I think you can click on "commit suggestion" or "add suggestion to batch" directly on GH to apply these, might be faster than manually doing each change (never tried it though so I'm not sure)

@voetberg
Copy link
Contributor Author

voetberg commented Apr 3, 2024

Some minor comments - btw, I think you can click on "commit suggestion" or "add suggestion to batch" directly on GH to apply these, might be faster than manually doing each change (never tried it though so I'm not sure)

Never noticed there was an option to batch them.... Always avoided committing from suggestions like this so I didn't clutter the commit history, but that's a nice feature. You win this one gh.

Co-authored-by: Riccardo Di Maio <35903974+rdimaio@users.noreply.github.com>
@rdimaio rdimaio self-requested a review April 3, 2024 18:18
rdimaio
rdimaio previously approved these changes Apr 3, 2024
@rdimaio
Copy link
Contributor

rdimaio commented Apr 3, 2024

Some minor comments - btw, I think you can click on "commit suggestion" or "add suggestion to batch" directly on GH to apply these, might be faster than manually doing each change (never tried it though so I'm not sure)

Never noticed there was an option to batch them.... Always avoided committing from suggestions like this so I didn't clutter the commit history, but that's a nice feature. You win this one gh.

Yeah, I thought it would be worth doing that and then doing "Squash and merge" when closing the PR, so that we'd end up with a single commit anyway, but it seems that it's not possible for this repo:

image

@bari12 is there a reason this feature is not available in this repo? It's available for the rucio/rucio repository

@rdimaio rdimaio dismissed their stale review April 3, 2024 18:22

Temporarily dismissing approval to see if we can enable "squash and merge" for this repo, so we can end up with a single commit

@rdimaio rdimaio merged commit 37c7421 into rucio:main Apr 4, 2024
6 checks passed
Comment on lines +80 to +81
query = session.execute(statement).scalars()
for column_a, column_b in query:
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR warranted additional discussion. This particular example isn’t even functional!

Copy link
Contributor

Choose a reason for hiding this comment

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

I re-opened #287, we can track follow-up comments on there and address them

rdimaio added a commit that referenced this pull request Apr 4, 2024
@rdimaio
Copy link
Contributor

rdimaio commented Apr 4, 2024

ignore this: fcdde46 - was just testing what GH did when you press revert on the PR

@rdimaio rdimaio mentioned this pull request Apr 4, 2024
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.

None yet

3 participants