Skip to content

Conversation

Huliiiiii
Copy link
Member

@Huliiiiii Huliiiiii commented Aug 7, 2025

PR Info

Enable clippy::pedantic

@Huliiiiii
Copy link
Member Author

Huliiiiii commented Aug 7, 2025

Still missing # Errors and # Panics docs.
I'm not sure whether I should add panic docs or remove panics.
Please take a look, @Expurple @tyt2y3.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 7, 2025

I think adding some panic docs is easiest right now. Converting panic to Error is not possible, unless ... we convert the API to return Result in build() etc, which is another big breaking change.

I honesty think we're running out of "innovation token" to make this change, but may be not!

Another thought is instead of panicking, we return some syntatically incorrect SQL, it will end up as an error.

@Expurple
Copy link
Member

I honesty think we're running out of "innovation token" to make this change

I agree. It feels like we should be wrapping up this SeaQuery release soon and moving on to SeaORM-specific tasks.

Another thought is instead of panicking, we return some syntatically incorrect SQL, it will end up as an error.

Sounds weird. I'd keep the panics for now and figure it out later

Copy link
Member

@Expurple Expurple left a comment

Choose a reason for hiding this comment

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

Still missing # Errors and # Panics docs.

IMO, # Errors would be useless noise in most cases. We should add # Panics, though

@Huliiiiii Huliiiiii force-pushed the clippy-pedantic branch 2 times, most recently from 7627db9 to 15db489 Compare August 11, 2025 12:06
@Huliiiiii
Copy link
Member Author

This is ready to be merged.

@Expurple
Copy link
Member

What about the semicolon check in CI? Looks good otherwise

@Expurple Expurple requested a review from tyt2y3 August 13, 2025 13:20
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.

3 participants