search

Nadrieril / dhall-rust

Maintainable configuration files, for Rust users
Other
303 stars 27 forks source link

feat: add cors validation #214

Closed kryptn closed 2 years ago

kryptn commented 3 years ago

I needed CORS validation for a project I'm working on

This is my first dive into rust and I'm not sure what else would be necessary to call this good to go, so I'm looking for feedback.

Thanks!

Nadrieril commented 3 years ago

Hi! Thanks for taking the time to do this, I never took the time. I'll trust you on the details, this looks pretty good! Did you manage to test this? I'd really like for there to be a test to ensure this keeps working in the future. I thought I remembered that the main dhall-lang repo had such a test but I can't find it.

Nadrieril commented 3 years ago

@Gabriel439 do you test CORS compliance on the Haskell codebase? I thought I remembered you setting up something on dhall-lang.org to help this testing, but I can't find it anymore.

Gabriella439 commented 3 years ago

@Nadrieril: I don't have automated testing around this, yet. You might be thinking of test.dhall-lang.org, which was created to test custom headers

Nadrieril commented 3 years ago

Oh right, that's what I got it mixed with. I see there's a relevant issue already (https://github.com/dhall-lang/dhall-lang/issues/1130), let's talk there

kryptn commented 3 years ago

Did you manage to test this? I'd really like for there to be a test to ensure this keeps working in the future.

No not yet, but absolutely agreed. I figured I'd seek feedback on the implementation itself first and then see what can be done with testing.

Nadrieril commented 3 years ago

I looked it over again and apart from the comment above I'm ok with the implementation :) Happy to merge it as soon as we figure out testing.

Nadrieril commented 3 years ago

I tried to run some test and it appears the implementation has a few errors. I added some comments

kryptn commented 3 years ago

Thanks! I'll get to resolving these.

Nadrieril commented 3 years ago

FYI latest commit on dhall-lang now has a decent set of CORS tests!

kryptn commented 3 years ago

Oh awesome! This will make validating the implementation much easier, thank you!

Nadrieril commented 2 years ago

Hey, how are you getting on with this? Are you still working on this? Would you like some help?

kryptn commented 2 years ago

I'll see where I can get with this over this upcoming weekend. iirc I might need some help with one of your suggestions, but I'll have to regain this context and likely update code.

kryptn commented 2 years ago

I don't think I'm going to finish this, the project I was working on that needed this didn't really work the way I wanted it to.

I won't get rid of my fork/branch though, just in case someone wants to take it over.

Nadrieril commented 2 years ago

No problem, thanks for letting me know! :)