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

Some fixes to stop confusion of kill delay value #397

Closed

Conversation

satetsu888
Copy link

@satetsu888 satetsu888 commented Mar 13, 2023

After reviewing several previous commits, I discovered that the method in which the kill_delay was specified led to some confusion.

In the initial implementation of kill_delay in commit #49, there was a confusing specification that indicated the value specified as an integer would be parsed as time.Duration and multiplied by time.Second. Therefore, specifying kill_delay as kill_delay = 2 would mean waiting for 2 * 1 second, but specifying kill_delay as kill_delay = "2s" would mean waiting for 2000000000 * 1 second.

In the subsequent commit ff8c2ed, an explanation (# ms) was added to air_example.toml, and a fix was made to multiply by time.Millisecond. Users who refer to air_example.toml can now use this option as expected, but unfortunately, those who execute air init will see a generated config file with kill_delay = "0s".

Then, commit #287 was introduced, and it seemed to aim to help users who configure their kill_delay as "1s". As a result, the configurations that refer to air_example.toml no longer work as expected.

The main topic of this pull request is to clarify the specification for kill_delay to avoid confusion. This involves fixing air_example.toml and some test values with kill_delay. It is now clarified that kill_delay should be a string such as "500ms".

Ideally, the WithArgs method should also be fixed to parse time.Duration, but I still believe this pull request is worthwhile!

@satetsu888
Copy link
Author

This pull request is closed because an other solution has been made in #462.

@satetsu888 satetsu888 closed this Mar 5, 2024
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.

1 participant