Skip to content

Updated documentation for setFullTime(long) #33

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

Closed
wants to merge 3 commits into from
Closed

Updated documentation for setFullTime(long) #33

wants to merge 3 commits into from

Conversation

BlameOmar
Copy link

setTime(long) is implemented using setFullTime(long). As long as the latter is not used to set the time to a point in the past, it should be as safe to use as the former. I'm uncertain as to what would happen if time were rewound, so I've adapted the advisory for that case.

Omar Stefan Evans added 2 commits July 15, 2013 17:22
Removed outdated notice of adverse effects on redstone clocks and scheduled events and clarified that it sets the time for the world, not the entire server which has multiple worlds.
Not certain what would happen if time were to rewind.  Restoring the advisory for setting time to points in the past.
@SagaciousZed
Copy link
Contributor

This breaks the formatting conventions, please fix.

@BlameOmar
Copy link
Author

I assume that I made a line too long. That's difficult to notice on the web interface; I'll make changes locally next time. I hope there are no other issues.

@Wolvereness
Copy link
Contributor

I don't think this change is appropriate; the current phrasing is sufficient to warn about possible behavior. This additionally carries a burden of proof as to if it's actual safe; you're effectively changing a contract with this new documentation.

So, in light of a more detailed documentation / justification, retaining our current documentation would be a better decision.

It's also worth noting that the hesitation here is more toward the "you're doing something unsafe!" -> "Oh, it's perfectly fine", and I hope it doesn't discourage future discourse or contributions.

@BlameOmar
Copy link
Author

The justification is in the implementation of setTime(long), which caries no such warning:

public void setTime(long time) {
        long margin = (time - getFullTime()) % 24000;
        if (margin < 0) margin += 24000;
        setFullTime(getFullTime() + margin);
}

setTime(long) calls setFullTime(long), passing the sum of a non-negative number an the current time as returned by getFullTime(). It follows that doing the same thing using setFullTime(long) directly would be at least as safe as calling setTime(long) multiple times in a loop as long as a number >= getFullTime() is passed.

As the only thing setFullTime(long) can do that setTime(long) ordinarily won't (the current implementation does not guard against overflow) is to "go back in time", which is what the warning should apply to.

I can amend the documentation to say something along the lines of "Take care to not set the time to a point in the past, or bad things may happen", which would better emphasize that using setFullTime(long) recklessly is potentially dangerous.

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.

3 participants