-
Notifications
You must be signed in to change notification settings - Fork 234
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
Fix regression in Kotlin error/exception names. #1842
Conversation
f1e700c
to
63d06e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I had one nit about the module visibility, but I think that can go either way.
@@ -401,43 +440,41 @@ pub mod filters { | |||
use super::*; | |||
pub use crate::backend::filters::*; | |||
|
|||
pub fn type_name(as_ct: &impl AsCodeType) -> Result<String, askama::Error> { | |||
pub(super) fn type_name(as_ct: &impl AsCodeType) -> Result<String, askama::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a big deal either way, but I'd lean towards keeping these pub
.
For one, it'll make the PR smaller, but I also generally prefer that functions are either public or private. If we don't want outside code to access these methods, then we can make gen_python
a private module. If we need more complicated access control, we can selectively import its public functions into the parent module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah - this was needed before I "unforked" otherwise I got errors about leaking private types. Note AsCodeType
is again pub I can remove this for python!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, I was wrong - I unforked that trait, but Python is still using the fork. Without this I get:
385 | trait AsCodeType {
| ---------------- `gen_python::AsCodeType` declared as private
...
443 | pub fn type_name(as_ct: &impl AsCodeType) -> Result<String, askama::Error> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak private trait
(which TBH I don't quite understand seeing as the filters module, while nominally pub
isn't actually exported anywhere.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and trying to make the trait itself pub then confuses even more things)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see, then pub(super)
makes sense to me.
This is still broken in 0.25.2 if the nested error is declared in an external crate.
This results in the type to be renamed from ApiRequestError to ApiRequestException but the import not reflecting this
|
Fixes #1828.
I'm frustrated with myself that I left this so broken - I think I was trying to avoid this larger Kotlin refactor that is sadly necessary. I ended up forking
CodeType
as only Kotlin needs error renaming.