-
Notifications
You must be signed in to change notification settings - Fork 824
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
Add support for creating Clients from a previously read settings file #181
Conversation
Hi @catsby, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
@catsby thanks for the pull request, looks like we need the CLA to be signed. My only comment is you can totally call Having duplicated logic is something we usually really want to avoid as people usually forget maintaining both at the same time. |
@catsby, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Hey @ahmetalpbalkan thanks for the reply
I've refactored some to consolidate and remove duplication, as well as preserve backwards compatibility. I think... can you take a look? Thanks! |
@catsby I think we can even remove duplication in
However if you could call
which really enforces a single path. Both are equally OK, however I find latter easier to read. :-) If you could also fix that, the rest is LGTM. |
Hey @ahmetalpbalkan , thanks for the continued help here. I hate to be difficult but I'm not sure I follow what you're suggesting, though I did just push 902e8bb to remove some duplication of
By this, do you mean for With 902e8bb , both |
LGTM. Thanks! |
Add support for creating Clients from a previously read settings file
Thanks so much @ahmetalpbalkan ! |
This adds support for initializing a
client
from the contents of a settings file that's stored in memory. At present,client
s are created by supplying a file path to a settings file, but for my use case I have the contents of the file stored elsewhere, either encrypted in a database or something like HashiCorp's VaultI originally tried modifying
ClientFromPublishSettingsFileWithConfig
to check thefilePath
parameter supplied, to see if it's valid XML and not a file path, but settled on duplicating and modifying theClientFromPublishSettingsFile
andClientFromPublishSettingsFileWithConfig
methods intoClientFromPublishSettingsData
andClientFromPublishSettingsDataWithConfig
, instead. There is duplication, but I felt it was simpler to reason about.Example usage, borrowing from the snippet in the README:
Suggestions on naming and testing are welcome.
Thanks!