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

Clarify the doc for Time.every #503

Merged
merged 1 commit into from Feb 18, 2016

Conversation

Projects
None yet
2 participants
@Janiczek
Contributor

Janiczek commented Feb 18, 2016

What unit is the first argument in (it is milliseconds).

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Feb 18, 2016

Contributor

Actually, saying "milliseconds" there is at odds with the advice further above in the documentation:

Using the Time constants instead of raw numbers is very highly recommended.

For example, for a call Time.every (3 * second) it seems strange to say that it is updated every "3 second milliseconds".

So I think the better change to the documentation here would be not to add "milliseconds" in the text, but instead replace the example expression every 100 in the next line by every (100 * millisecond).

Put differently, the "milliseconds" is an implementation detail. It could be changed at any time (without anything in the current documentation becoming wrong). If people keep to the advice to use the time constants, such a change will have no effect on them. But your documentation proposal would become false then.

Contributor

jvoigtlaender commented Feb 18, 2016

Actually, saying "milliseconds" there is at odds with the advice further above in the documentation:

Using the Time constants instead of raw numbers is very highly recommended.

For example, for a call Time.every (3 * second) it seems strange to say that it is updated every "3 second milliseconds".

So I think the better change to the documentation here would be not to add "milliseconds" in the text, but instead replace the example expression every 100 in the next line by every (100 * millisecond).

Put differently, the "milliseconds" is an implementation detail. It could be changed at any time (without anything in the current documentation becoming wrong). If people keep to the advice to use the time constants, such a change will have no effect on them. But your documentation proposal would become false then.

@Janiczek

This comment has been minimized.

Show comment
Hide comment
@Janiczek

Janiczek Feb 18, 2016

Contributor

@jvoigtlaender That makes sense. I agree about replacing every 100 by something more clear. It begs the question "100 of what?" which I wanted to answer in the doc but is really better answered in the code by using an "unit."

Contributor

Janiczek commented Feb 18, 2016

@jvoigtlaender That makes sense. I agree about replacing every 100 by something more clear. It begs the question "100 of what?" which I wanted to answer in the doc but is really better answered in the code by using an "unit."

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Feb 18, 2016

Contributor

Cool. If you squash the two commits into one, I will merge.

Contributor

jvoigtlaender commented Feb 18, 2016

Cool. If you squash the two commits into one, I will merge.

Clarify Time.every
What unit is the first argument in (it is milliseconds).
@Janiczek

This comment has been minimized.

Show comment
Hide comment
@Janiczek

Janiczek Feb 18, 2016

Contributor

I hope force push was the right thing to do :D

Contributor

Janiczek commented Feb 18, 2016

I hope force push was the right thing to do :D

jvoigtlaender added a commit that referenced this pull request Feb 18, 2016

Merge pull request #503 from Janiczek/patch-1
Clarify the doc for Time.every

@jvoigtlaender jvoigtlaender merged commit fa64500 into elm:master Feb 18, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Janiczek Janiczek deleted the Janiczek:patch-1 branch Feb 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment