-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
Changed manual url parsing to use Url crate #253
Changed manual url parsing to use Url crate #253
Conversation
Hey @AngelOnFira, welcome to the party! |
Hey! 👋 I'm excited to get started, I'm really loving how SeaORM is working behind the scenes so far 😃 |
Also, could I make tests for the CLI? It would be helpful to test that database URLs are valid. I'm not exactly sure how I'd go about it, but I'm sure there are Rust tests that could call the CLI somehow? |
Yes I think it's possible to test it cli level, though a lot of setup has to be done. Unit tests would be sufficient for the scope of this PR. So it's up to you to decide what's interesting to work on. |
Ok, I've added the tests, so I think this is good to go! Note; I also updated the Github Actions file for them to run in CI. Any comments for how anything is done would be nice, I think I'm at the point where I'm happy with how it's implemented 💯 |
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.
https://github.com/SeaQL/sea-orm/pull/253/checks?check_run_id=3935411921#step:6:252
Seems to be passing! Nice work all in all!
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.
Downloaded & Tested! Nice PR :P
Thanks!! @AngelOnFira
Congrats on landing the first |
Thanks 😄 |
This changes the manual parsing of database url/uri to instead use the Url crate.