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

Run black on codebase #765

Merged
merged 9 commits into from
Apr 17, 2024
Merged

Run black on codebase #765

merged 9 commits into from
Apr 17, 2024

Conversation

Harsha-Nori
Copy link
Collaborator

Running black on the codebase to standardize things and set us up for auto formatting as our contributor base grows.

Following git/github's conventions with a .git-blame-ignore-revs file to not affect git blame: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

Also fixes a small merge issue in #759 by deprecating guidance.newline, which is causing a notebook test failure.

I believe isort is still causing some issues with circular imports, so not enforcing that entirely for now.

guidance/models/_openai.py Dismissed Show dismissed Hide dismissed
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 61.82965% with 242 lines in your changes are missing coverage. Please review.

Project coverage is 65.68%. Comparing base (4b45025) to head (98535df).

Files Patch % Lines
guidance/_serialization_pb2.py 6.89% 27 Missing ⚠️
guidance/models/_grammarless.py 43.47% 26 Missing ⚠️
guidance/models/vertexai/_vertexai.py 11.11% 24 Missing ⚠️
guidance/models/_model.py 84.34% 18 Missing ⚠️
guidance/models/_googleai.py 33.33% 16 Missing ⚠️
guidance/models/_lite_llm.py 15.78% 16 Missing ⚠️
guidance/_grammar.py 72.00% 14 Missing ⚠️
guidance/models/_openai.py 43.47% 13 Missing ⚠️
guidance/_parser.py 81.66% 11 Missing ⚠️
guidance/library/_gen.py 47.05% 9 Missing ⚠️
... and 22 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #765      +/-   ##
==========================================
- Coverage   65.68%   65.68%   -0.01%     
==========================================
  Files          55       55              
  Lines        4060     4059       -1     
==========================================
- Hits         2667     2666       -1     
  Misses       1393     1393              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -11,6 +11,7 @@ node_modules
/client
.eggs/
.env
.DS_Store
Copy link
Collaborator

Choose a reason for hiding this comment

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

Someone has a Mac....

Comment on lines +97 to +102
blanks = (
"\n" * f.__code__.co_firstlineno
) # padd the source so the lines in the file line up for the debugger
source = blanks + "\n".join(
source.splitlines()[1:]
) # remove the decorator first line.
Copy link
Collaborator Author

@Harsha-Nori Harsha-Nori Apr 17, 2024

Choose a reason for hiding this comment

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

in general, I'm not a fan of how black handles long-ish lines with trailing comments. I suppose I'd prefer that it moves trailing comment lines to the preceding line, and then keep the source as a single compact line. But a standard is a standard 🤷‍♂️ . Would be nice for the future if there's a way to configure this better though, as I think:

 # padd the source so the lines in the file line up for the debugger
 blanks = "\n" * f.__code__.co_firstlineno

looks much nicer than

 blanks = (
        "\n" * f.__code__.co_firstlineno
    )  # padd the source so the lines in the file line up for the debugger

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@riedgar-ms @ryanpeach curious about your thoughts here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would agree with your preference, although it's more something that could be fixed up as people review the code afterwards rather than needing to go through all the files now.

@Harsha-Nori Harsha-Nori merged commit 6167225 into main Apr 17, 2024
83 checks passed
@ryanpeach
Copy link
Contributor

Much appreciated!

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