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

refactor!: identifier to ident and update docs #441

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

varshith257
Copy link
Contributor

@varshith257 varshith257 commented Dec 17, 2024

Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.

Please go through the following checklist

Rationale for this change

To update docs to reflect the migration of Identifier -> Ident

What changes are included in this PR?

  • Updated docs in the proof-of-sql crate with Ident

Are these changes tested?

Part of #235

Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

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

Really thanks for your PR!
General comments:

  1. You don't need to remove the word "identifier" when it refers to the general concept of identifiers rather than proof_of_sql_parser::Identifier
  2. .The comments on the two table util files need to be fixed in the entire files rather than in just the two instances I mentioned.

@varshith257 varshith257 changed the title docs: update docs of identifier to ident refactor!: identifier to ident and update docs Dec 17, 2024
@varshith257 varshith257 force-pushed the update-docs branch 2 times, most recently from 9340da5 to ef5c2f1 Compare December 17, 2024 19:55
@iajoiner iajoiner self-requested a review December 17, 2024 20:01
Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

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

Please remove unnecessary panic docs from owned_table_utility.rs and table_utility.rs

@varshith257
Copy link
Contributor Author

The doc test is failing if we do the same and getting clippy issue there

@iajoiner
Copy link
Contributor

@varshith257 Which issues?

@varshith257 varshith257 force-pushed the update-docs branch 2 times, most recently from db11bb7 to 2c1812f Compare December 18, 2024 03:05
Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR!

@iajoiner iajoiner merged commit c83b298 into spaceandtimelabs:main Dec 18, 2024
12 of 13 checks passed
@varshith257 varshith257 deleted the update-docs branch December 18, 2024 15:44
Copy link

🎉 This PR is included in version 0.64.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants