plaid
Make the environment a struct
Proposal to address #114, #109 introduced a change to the Environment constants, however if you were passing in static strings into the client, #109 would have broken things
This makes it so that at least the compiler will complain if a string is being passed..
@nathankot I'm a bit torn here. This definitely would solve for the "principle of least surprise" dimension, but this breaks a use-case for us, which is being able to externalize the configuration of which Plaid environment we use from the application.
It's not a big deal, but not being able to pass in strings means we'll need to wrap our environment selection in a switch statement that maps from some external config value down to the exposed consts. So in a way, this is another backwards-incompatible change for us, though not one that will catch us by surprise ;-).
Either path you choose, thank you for looking at #114 and CCing me here!
@bmhatfield ah how interesting! For some background, the #109 change was made so that we could pass our own environment URLs in development, before #109 the client library would error out on initialization if the url string that it sees did not match one of the known values, #109 made it so that it just logs a warning.
Just realized our use-case is similar to yours where we want to pass in a custom string for development, I updated this PR so that Environment.url becomes publicized as Environment.URL so you can pass in Environment{ URL: <string> }, wondering if that will work for you?
There was also a previous version of #109 (4b6adf2) that would fail by default if the environment was not known, but you could pass a flag to disable that check, starting to think that might have been better 🤔
Just realized our use-case is similar to yours where we want to pass in a custom string for development, I updated this PR so that Environment.url becomes publicized as Environment.URL so you can pass in Environment{ URL:
}, wondering if that will work for you?
From a technical standpoint, that will work just fine. However, I think it loses some of the benefits of the compile time checking the unexported version provides. After thinking about it for a little bit, it does not feel materially different than the current approach of wrapping string, but I may be missing important context.
To back up a step - I filed #114 to make it was visible that the change was not backwards compatible, since I had a hunch that it was not obvious that #109 would impact users of the library. We have since adjusted our code to the new reality. From my perspective, everything is currently in a steady-state and no further changes are needed. We are happy users.
I might gently encourage a test or comments in the code explaining that users can optionally pass their own string values rather than using the exported consts. I've added a test on my side to make sure that our configs pass Environment.Valid()
There was also a previous version of #109 (4b6adf2) that would fail by default if the environment was not known, but you could pass a flag to disable that check, starting to think that might have been better
I suspect your instinct is right. The warning message did print, but our app logs other stuff on startup and I missed it when I was testing out my updates to go.mod. Outright failing would have been more directly obvious as a user. It may even help other people who are doing the same thing we are, but haven't updated yet.
Thank you again for taking the time to reason through this issue and propose changes!
@nathankot any action item here or should we close this and https://github.com/plaid/plaid-go/issues/114?
