Skip to content

Move the keywords module #352

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

Merged
merged 5 commits into from
Sep 26, 2021
Merged

Move the keywords module #352

merged 5 commits into from
Sep 26, 2021

Conversation

koushiro
Copy link
Member

Signed-off-by: koushiro [email protected]

@coveralls
Copy link

coveralls commented Sep 10, 2021

Pull Request Test Coverage Report for Build 1275068126

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.206%

Totals Coverage Status
Change from base Build 1273541769: 0.0%
Covered Lines: 5821
Relevant Lines: 6453

💛 - Coveralls

@koushiro
Copy link
Member Author

/cc @alamb @Dandandan

@alamb
Copy link
Contributor

alamb commented Sep 15, 2021

So something that would help me understand these PRs is the rationale for making the change. For example, this PR perhaps makes the code structure a bit more logical but won't it require a bunch of rework for all users of the library?

@koushiro
Copy link
Member Author

this PR perhaps makes the code structure a bit more logical but won't it require a bunch of rework for all users of the library?

yes

@houqp
Copy link
Member

houqp commented Sep 26, 2021

perhaps we can reexport the keywords module in dialect module to make it backwards compatible until we do the next major release?

@koushiro
Copy link
Member Author

perhaps we can reexport the keywords module in dialect module to make it backwards compatible until we do the next major release?

@alamb has been released v0.11

@alamb
Copy link
Contributor

alamb commented Sep 26, 2021

I guess I am just thinking of users of this crate -- when they upgrade to a new version of sqlparser I feel the best experience is to minimize the amount of changes they have to make to their code.

For changes required to support new features (e.g. new enum variants or new fields in existing variants) I don't see a way around requiring the user code to change.

However, I don't like the idea of causing work for downstream users for changes that are basically internal improvements to sqlparser-rs

Thus, I would be happy to merge this PR if it was backwards compatible (and @houqp has suggested a good way to do that)

@koushiro
Copy link
Member Author

@alamb Now the change is backwards compatible, I originally planned to add deprecated to re-export, like below:

#[deprecated(
    since = "0.11.1",
    note = "The keywords module has been moved into the root of the library, \
    please use sqlparser::keywords instead of sqlparser::dialect::keywords"
)]
pub use crate::keywords;

but it doesn't seem to work, see rust-lang/rust#30827

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @koushiro !

@alamb alamb merged commit c9f8a44 into apache:main Sep 26, 2021
@koushiro koushiro deleted the move-keywords branch September 26, 2021 12:18
@coveralls
Copy link

coveralls commented Sep 18, 2024

Pull Request Test Coverage Report for Build 1272354313

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.206%

Totals Coverage Status
Change from base Build 1270918470: 0.0%
Covered Lines: 5821
Relevant Lines: 6453

💛 - Coveralls

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