Skip to content

Add stub for thrift_library()#10

Closed
ben-- wants to merge 1 commit intofacebook:mainfrom
ben--:pr10
Closed

Add stub for thrift_library()#10
ben-- wants to merge 1 commit intofacebook:mainfrom
ben--:pr10

Conversation

@ben--
Copy link
Copy Markdown
Contributor

@ben-- ben-- commented Jun 23, 2025

No description provided.

@ben-- ben-- marked this pull request as ready for review June 23, 2025 21:51
shims.bzl Outdated
Comment on lines +476 to +480
for l in languages:
if False:
pass
else:
print("FIXME(buck2-shims-meta): unsupported thrift languages: {}".format(", ".join(unsupported)))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this do something simpler? Why if False? Why log each language if we don't support any of them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The way I've seen similar code implemented, it loops over languages and has a bunch of

if l == "cpp":
    gen_for_cpp()
elif l == "rust":
    gen_for_rust()
else:
    print("unsupported language: {}".format(l))

The log message was helping me keep track of what else I had to do as I tried an open-source implementation of rust.

Happy to just make this log an error message if you'd prefer. Let me know.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually wait, does this code work? When is unsupported ever written to? Shouldn't you format with l instead?

@ben-- ben-- requested a review from bigfootjon June 24, 2025 00:14
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@bigfootjon has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

shims.bzl Outdated
Comment on lines +476 to +480
for l in languages:
if False:
pass
else:
print("FIXME(buck2-shims-meta): unsupported thrift languages: {}".format(", ".join(unsupported)))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually wait, does this code work? When is unsupported ever written to? Shouldn't you format with l instead?

@ben--
Copy link
Copy Markdown
Contributor Author

ben-- commented Jun 24, 2025

Code fixed. Sorry about that -- was moving too fast.

@ben-- ben-- requested a review from bigfootjon June 24, 2025 16:05
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@bigfootjon has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit to facebook/ocamlrep that referenced this pull request Jun 25, 2025
Summary: Pull Request resolved: facebook/buck2-shims-meta#10

Reviewed By: sdwilsh

Differential Revision: D77232708

Pulled By: bigfootjon

fbshipit-source-id: 590a758856fbaf1d570df8c9161dd6c4c4123e96
facebook-github-bot pushed a commit that referenced this pull request Jun 25, 2025
Summary: Pull Request resolved: #10

Reviewed By: sdwilsh

Differential Revision: D77232708

Pulled By: bigfootjon

fbshipit-source-id: 590a758856fbaf1d570df8c9161dd6c4c4123e96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants