Skip to content

Conversation

eitsupi
Copy link
Member

@eitsupi eitsupi commented Feb 9, 2023

Currently, the sql. prefix have to be removed from the target name and pass to Dialect::from_str.
This PR implements Target::from_str and does this on the Target::from_str side, eliminating the need for downstream software to implement this process.

Example of Use: PRQL/prqlc-r#78

@eitsupi eitsupi marked this pull request as draft February 9, 2023 12:36
@eitsupi eitsupi marked this pull request as ready for review February 9, 2023 14:49
@eitsupi
Copy link
Member Author

eitsupi commented Feb 9, 2023

As far as trying in PRQL/prqlc-r#78, I think this is working fine.

But obviously tests on these should be added here.
I am not familiar with Rust testing and would like to add tests at another time...

Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Thanks a lot @eitsupi — great that you're diving into the rust!

Would it be possible to add a short test in the file? Let me know if an example would be helpful.

@max-sixty
Copy link
Member

I am not familiar with Rust testing

It's surprisingly easy!

We can just have something like this, in the file:

#[cfg(test)]
mod test {
use insta::assert_debug_snapshot;

#[test]
fn test_target_from_str() {

    assert_debug_snapshot!(Target::from_str("sql.postgres".to_string()), @"");

    assert!(Target::from_str("sql.poostgres".to_string().is_err());
}
}

...then running cargo insta test --accept will fill out the @ string.

@eitsupi eitsupi marked this pull request as draft February 10, 2023 13:42
@eitsupi eitsupi marked this pull request as ready for review February 10, 2023 14:45
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.

2 participants