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

[Bug] Different output for scoop config last_update depending on shell #5229

Closed
StormPooper opened this issue Oct 28, 2022 · 16 comments · Fixed by #5247
Closed

[Bug] Different output for scoop config last_update depending on shell #5229

StormPooper opened this issue Oct 28, 2022 · 16 comments · Fixed by #5247
Labels

Comments

@StormPooper
Copy link

StormPooper commented Oct 28, 2022

Bug Report

Current Behavior

When I run scoop config last_update in Powershell 7 (pwsh) or the Command Prompt (cmd), I get a formatted date, e.g:

PS > scoop config last_update

28 October 2022 1:35:03 PM

When I run the same command in Windows Powershell (v5, powershell), I get the same format as in config.json:

PS > powershell scoop config last_update
2022-10-28T13:35:03.516507+01:00

Expected Behavior

Regardless of the shell, I would expect the date to be the same format as in config.json.

Additional context/output

The value in config.json is maintaining the same format in config.json when scoop is run in Powershell 7, it only appears to be scoop config last_update that is impacted. I've been using the output to avoid triggering changes in my dotfiles, but since the format is different to the file content it sees a difference in the file.

System details

Windows version: 11 2H22

OS architecture: 64bit

PowerShell version: 7.2.7

Windows Powershell version: 5.1.22621.608

Scoop Configuration

{
  "last_update": "2022-10-28T13:35:03.516507+01:00",
  "aria2-warning-enabled": false,
  "scoop_repo": "https://github.com/ScoopInstaller/Scoop",
  "scoop_branch": "master"
}
@niheaven
Copy link
Member

Sorry it's PowerShell's display rule, and is not scoop's scope. Just read config file directly should help.

@StormPooper
Copy link
Author

I initially thought that too, but when I do [System.DateTime]::Now.ToString("o") in pwsh I get the correct value printed, which is what the code does to update the value. I think the issue is that scoop-config just does $value when printing the value, which I can reproduce like so:

PS > $value = [System.DateTime]::Now
PS > $value
Friday, 28 October 2022 16:28:56

PS > $value.ToString("o")
2022-10-28T16:28:56.9216980+01:00

It seems like the fix would be for scoop-config to do ToString("o") to ensure the output is consistent with the contents of the file.

@niheaven
Copy link
Member

In fact that value is internal used, and not advised to set or get by users... Why not read the file?

@StormPooper
Copy link
Author

I can do, but I wouldn't be surprised if I'm the only one who's been relying on this for scripting reasons and figured it might be useful for others if this was fixed (plus it's easier to just read the output of the command directly), so thought I'd raise it. Not a big deal though.

@niheaven
Copy link
Member

Hmm, in fact for time displayed, scoop should output human readable style, just like the one in scoop hold scoop

image

The time is read from hold_update_until in config.json, and its output is the same in PS5 and PS7.

@StormPooper
Copy link
Author

Looks like scoop_hold does .ToLocalTime() on the value when printing it out, which would be why it's consistent across both versions, whereas scoop_config doesn't specify a format so is at the mercy of the shell defaults.

@rashil2000
Copy link
Member

scoop_config doesn't specify a format so is at the mercy of the shell defaults

This is on purpose, because the raw output gives users freedom to deal with it however they want. If you'd like it to output consistently you can specify an output format.

@StormPooper
Copy link
Author

StormPooper commented Oct 29, 2022

If you'd like it to output consistently you can specify an output format.

Can you give me an example of this? I've tried using ToString("o") on the output as per the rest of the code and my example with the variable advice, but it doesn't make a difference (I could be doing it wrong, but this was part of the reason I raised this).

Edit: I found I can do (scoop config last_update).ToString("o") in Powershell 7, though this doesn't work in Windows Powershell. I still think it would be better for scoop to return a consistent and predictable output as scripts will have to account for the version of Powershell (or even worse, running from the Command Prompt) and I think it's reasonable to assume that the value returned from the config command would match what is in the JSON file it is reading from. It's also worth noting that Scoop-Config.Tests.ps1 covers this exact scenario and they pass locally in Powershell 7, so there's a disconnect between what the tests see and what is actually output to the shell.

@niheaven
Copy link
Member

As I said, last_update is an internal config option and is not designed to be used externally. If you really need this value, a better solution is to read the file directly, to avoid any regional variation, and it is faster and more reliable.

@StormPooper
Copy link
Author

That's fine, I already have a workaround for my original usage, so I'm happy. I just thought it was worth bringing up and explaining my rationale for why I think it's worth fixing. If we disagree on whether it is worth fixing then fair enough, I won't invest any time creating a PR to fix it.

@niheaven
Copy link
Member

niheaven commented Oct 29, 2022

I still wonder why you need that value... Is there some special usage that we haven't found? Or is there some existed feature that could fit it but is hard to find?

@StormPooper
Copy link
Author

As I mentioned in the original post, I'm using the value as part of my dotfiles management to sync config across multiple machines. I use chezmoi and I'm using the output of scoop config last_update to regenerate the template so that chezmoi doesn't see the updated date as a value that needs updating/resetting:

{
  "last_update": "{{ output "scoop" "config" "last_update" | trim }}",
  "aria2-warning-enabled": false,
  "scoop_branch": "master",
  "scoop_repo": "https://github.com/ScoopInstaller/Scoop"
}

My current workaround is to change the template to use output "powershell" "scoop" "config" "last_update" and relying on WIndows Powershell being available, which it is for any machine where I'd be using Scoop, so not a big deal. The alternative is to write a custom script, but this is simpler in this case. I can't find where I originally found the suggestion to use last_update, I believe it was in another dotfile management tool's issue tracker, but it was at least a year ago now.

@niheaven
Copy link
Member

Gotta, may fix in next release.

@StormPooper
Copy link
Author

That'd be great, no worries if not though. I think the logic in the config script should be simple enough to fix, but I was unsure how to write a test to cover the change as the existing tests check the format and pass, even when I run them locally in Powershell 7.

@rashil2000
Copy link
Member

Can you give me an example of this? I've tried using ToString("o") on the output as per the rest of the code and my example with the variable advice, but it doesn't make a difference (I could be doing it wrong, but this was part of the reason I raised this).

scoop config last_update | get-date will give you the same output on PowerShell 5.1 and 7.

@StormPooper
Copy link
Author

scoop config last_update | get-date will give you the same output on PowerShell 5.1 and 7.

Thanks, I can do (scoop config last_update | Get-Date).ToString("o") in both to get the same format as the config file uses. Unfortunately, chezmoi's template doesn't appear to support the more complicated syntax, so I'll have to stick with my workaround for now, but I appreciate the answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants