Skip to content

Conversation

@Gauravms2143
Copy link

@Gauravms2143 Gauravms2143 commented May 24, 2023

When we launch the mock server:
Earlier Behaviour:

  1. For the very first time and if the imposters folder doesn't exist, it will give an error message and fail to launch.
    enhance
  2. create a default imposter folder manually.

New approach:

  1. Even if the imposters folder doesn't exist at the very first time, create it on the fly.

Test Case:
I removed the first test case from server_test.go.
If we use second approach there is no need to check if the imposters folder exists or not.

@joanlopez
Copy link
Member

joanlopez commented May 24, 2023

Hey @Gauravms2143,

First of all thanks for your contribution, it's really nice to see new faces contributing to the project.

However, I'd love to understand what's the goal of this pull request and which are the pains it's trying to solve, before digging into the code details. Could you extend the description with some additional context?

Here's some initial thoughts I have:

  • I don't think it actually solves any issue because:
    • If you specified the wrong path by mistake, creating a new directory wouldn't solve the issue, but hide it.
    • If you don't have any imposter at all, creating the empty directory/file won't help without any other manual action.

Which makes me think that:

  • Perhaps what we actually need is a command to help with the generation of new imposters.
  • Once we get this merged, it may start to make some sort of sense, because the user may want to start recording from scratch, but even in such case I have doubts about how the user experience should be.
  • Overall, I'm concerned it may cause undesired effects, like creating empty directories here and there with no purpose or just by mistake (in case the user specifies a wrong path).

cc/ @aperezg any additional thoughts?

Thanks!

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