cjstehno / ersatz

🤖 A simulated HTTP server for testing client code with configurable responses.
https://cjstehno.github.io/ersatz
Apache License 2.0
47 stars 5 forks source link

consider switching to uppercased method names in 2.x #110

Closed musketyr closed 4 years ago

musketyr commented 5 years ago

using method get causes a problem within DSLs as it also allows to use unknown properties. instead of property missing exception, you get an expectation. consider naming the method GET instead and rename all the other methods to use uppercased characters.

cjstehno commented 5 years ago

Hmm. I actually don't use missing method or properties, it's all coded and the in 2.0 it's going to be written in pure Java. I'll consider the upper case method names but maybe as an option rather than the only way. Thanks.

musketyr commented 5 years ago

It doesn't actually matter whether it is intended or not. Any missing property is translated into get('propertyName'). Even worse, as the property resolution is currently broken, the properties present in spec (except local variables) also resolved using the get method.

Dne út 20. lis 2018 18:15 uživatel Christopher J. Stehno < notifications@github.com> napsal:

Hmm. I actually don't use missing method or properties, it's all coded and the in 2.0 it's going to be written in pure Java. I'll consider the upper case method names but maybe as an option rather than the only way. Thanks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cjstehno/ersatz/issues/110#issuecomment-440354326, or mute the thread https://github.com/notifications/unsubscribe-auth/AAoTtWwLsGybftBZUWBctr2pr3-ydXKBks5uxDicgaJpZM4YrRzl .

cjstehno commented 5 years ago

I guess I'm not fully understanding what you mean. I'll take a deeper look at this after this holiday week when my brain is back "on". 🦃

musketyr commented 5 years ago

no worries, the exact situation is actually described in https://github.com/cjstehno/ersatz/issues/108

cjstehno commented 5 years ago

Ok, this all makes sense now. I'm not really a fan of the all-caps methods but I'll either deal with it or come up with something better. Thanks for pointing this out.

cjstehno commented 5 years ago

When I was coming up with the original design for the DSL I considered using a more generic approach for the request methods, something like:

server.expectations {
    request(GET, '/foo'){
        // inside is similar to existing...
    }
    request(HEAD, '/bar'){
        // ...
    }
}

It still has a nice, expressive look to it and it would reduce the somewhat-duplicated methods in Expectations down to a couple. The GET in this case would be an enum with a value for each supported HTTP method type.

With the alternative being your suggestion of capitalized methods, such as:

server.expectations {
    GET('/foo'){
        // inside is similar to existing...
    }
    HEAD('/bar'){
        // ...
    }
}

Syntactically, the two are not all that different, but I think I may lean towards the first approach mainly for its reduction of code.

What are your thoughts on this?

musketyr commented 5 years ago

I'm definitely against it ;-) it's good to also expose the genetic method but having the methods for each http method is very convenient.

KrzysztofKowalczyk commented 5 years ago

I had problem with get(String) in my dsl. There is simple workaround for missing property resolution. Instead of

def get(String path) {}

Use

def get(CharSequence path) {}

This way the API stays the same, but it is not considered a missing property getter.

Having different names allow one to define different types for closure delegates, hence you can have GET request not allowing body for expectations but have it with POST, etc.

cjstehno commented 5 years ago

@KrzysztofKowalczyk Interesting, I will definitely look at that as an alternative to the uppercase names. Thanks.

musketyr commented 5 years ago

thanks @KrzysztofKowalczyk I'll check this in my DSLs as well

cjstehno commented 4 years ago

Ok, it took some time, but I am warming up to this change. I have added the all-caps methods in 1.10 with the old lower-case ones deprecated - to be removed in 2.0.