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

Updated uptime segment to have show time in a better way #2036

Merged
merged 4 commits into from Oct 5, 2020

Conversation

StopMotionCuber
Copy link
Contributor

This mainly addresses #1859 and provides a reasonable standard way of showing the uptime.

Now, this format

  • Does show a time units with 0 as long as there are bigger time units (e.g. will show 0 seconds if there are minutes but not days if your uptime is below a day)
  • Shows 2 digits for seconds and minutes and thereby prevents frequent changes in size here (I didn't add that for hours because I think these 2 size changes/day are not distracting)

I would be happy if this could get merged and finds it way into the repo

@PH111P
Copy link
Member

PH111P commented Oct 25, 2019

Thanks for your work @StopMotionCuber!

I have some comments:

  • While the current (default) configuration is indeed not really optimal, your change is not backwards-compatible: If one previously configured the different formats to have leading zeros (as you did with the new default; e.g. as minutes_format=' {minutes:02d}m')', then now the segment will not work anymore, as the format string is not supplied with the field minutes anymore.
    While I'm not against breaking changes in general, this one in particular seems not necessary in order to achieve what you try to achieve.
  • In general, I suggest that we add a new parameter format (either as a python format string or as a strftime string) that is supplied with all the different values (days, minutes, ...). This should be far more general than the current solution.
  • In any case, the tests for this segment need to be updated.

@StopMotionCuber
Copy link
Contributor Author

That's true

Mainly I did that because code gets more repetitive if you have to assign every Keyword Argument.
My question is now what is your idea of the format parameter?
I have multiple ideas that I could go on with:

  • Leave that out completely and change to a backwards compatible solution
  • Add format as a new parameter additionally to the old parameter. This would mean that we actually have 2 solutions here and only use the backwards-compatible one if arguments for this are provided
  • Omit backwards compatibility for the sake of only having one format string

I would prefer approach 3 as I doubt that many people have a custom format here anyway and this would lead to a clean, maintainable solution, but I'd be fine with all 3 approaches

@PH111P
Copy link
Member

PH111P commented Oct 25, 2019

Personally, I'd prefer the following solution:

  • Add a new format parameter that gets all the different values; and add all different values to the old formats as well. This would mean that we have the same parameter essentially 5 times. While this would be kind of redundant, it is backwards compatible and allows for clean code.

@StopMotionCuber
Copy link
Contributor Author

StopMotionCuber commented Oct 25, 2019

I made the current solution now backwards compatible, but didn't add a new format (yet), so no need to merge already.

The problems that I see for supporting a new for format while maintaining backwards compatibility is:

  • How do we set the default arguments?
  • How to detect if we are in backwards-compatible mode or using the new format string
  • What is the behavior if a user has set only certain formats (e.g. only custom days_format, but the rest is still default)

While all of this is possible to implement, I don't want to have a hacky if/else solution that covers every of these edge cases while bloating the code just for the sake of backwards compatibility.

Speaking of backward compatibility, why do we want to keep this?
To keep custom formats or to not break existing configurations?

If it's only the latter, I'd recommend to keep these parameters and make them non-functional (or, even better, replace them by a non-functional keyword-arguments).

@PH111P
Copy link
Member

PH111P commented Oct 27, 2019

Adding a new format parameter could be done e.g. by letting it default to None. If a non-None value is present, format will be used, otherwise it won't.
As all of the other arguments already have default arguments, the case where some of them are not specified just isn't relevant: If a user only configures some of them, the default argument will be used for the arguments not specified.

Note that before your changes it was indeed possible to not show some of the values—by setting one of the format parameters to None. Setting a format parameter to None with your code will now result in a crash.

@PH111P
Copy link
Member

PH111P commented Oct 27, 2019

Looking at the original code again, I realized that there is actually no need for a general format parameter: Setting a specific format string to null in the config removes it from the output; shorten_len can be used to show at most 3 values at any time.
So the only change required would be to show values that are 0 and that come after a non-zero value. For this, I propose the following code (which does not crash if some of the formats are missing):

    minutes, seconds = divmod(seconds, 60)
    hours, minutes = divmod(minutes, 60)
    days, hours = divmod(hours, 24)
    time_formatted = list(filter(None, [
        days_format.format(days=days) if days_format else None,
        hours_format.format(hours=hours) if hours_format else None,
        minutes_format.format(minutes=minutes) if minutes_format else None,
        seconds_format.format(seconds=seconds) if seconds_format else None,
    ]))
    first_non_zero = next((i for i, x in enumerate([days, hours, minutes, seconds]) if x != 0))
    time_formatted = time_formatted[first_non_zero:first_non_zero + shorten_len]
    return ''.join(time_formatted).strip()

@StopMotionCuber
Copy link
Contributor Author

Left this PR hanging for a rather long time, I included your code now @PH111P and I think this should be mergable if the tests pass (Otherwise I'll take a look at those tests)

@PH111P PH111P merged commit a80bbdf into powerline:develop Oct 5, 2020
@StopMotionCuber StopMotionCuber deleted the better_uptime branch October 5, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants