Skip to content
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

Adding S3 Compatible Storage Support and Changing "timeout" to "frequency" #2

Merged
merged 10 commits into from
Sep 21, 2020

Conversation

nehrman
Copy link
Contributor

@nehrman nehrman commented Sep 18, 2020

Hi,
So I recreated a PR without the binary and with the last changes I made about timeout.
My customer asked to change this as this was not very clear to him and he prefers to have "frequency" instead.

@Lucretius
Copy link
Owner

Lucretius commented Sep 18, 2020

In general I probably won't want to rename properties based on a particular individuals desires because it would lead to a lot of churn for any existing users of the application - but in this particular case, I think renaming timeout to frequency (or interval or something like that) makes sense. I'll see if I can get this merged in today.

@Lucretius
Copy link
Owner

@nehrman Are you able to run the Go formatter on your PR? It looks like the frequency property change is off-shifted in config.go

@@ -13,7 +13,7 @@ import (
type Configuration struct {
Address string `json:"addr"`
Retain int64 `json:"retain"`
Timeout string `json:"freq"`
Frequency string `json:"frequency"`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the formatter needs to be run here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good

@Lucretius Lucretius merged commit cae1ee2 into Lucretius:master Sep 21, 2020
@Lucretius
Copy link
Owner

Thank you for the PR. I've gone and merged it, and built a new release (v0.1.0) for you to use.

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