Skip to content

Removes spaces before unit.#23

Merged
thaJeztah merged 1 commit intodocker:masterfrom
wernight:master
Jan 27, 2017
Merged

Removes spaces before unit.#23
thaJeztah merged 1 commit intodocker:masterfrom
wernight:master

Conversation

@wernight
Copy link
Contributor

This is to be closer to common Linux outputs.
See moby/moby#30271

@thaJeztah
Copy link
Member

Could you update the commit message to provide some background for this change (i.e., describe the problem with sort without this change)?

also ping @LK4D4 @justincormack PTAL

@stevvooe
Copy link

LGTM

@thaJeztah Sort breaks because it treats them as separate fields in the comparison.

This may break some tests in docker.

@thaJeztah
Copy link
Member

This may break some tests in docker

@stevvooe Yes, tests will have to be updated when this is vendored. Do you think this will be too much of a breaking change?

@stevvooe
Copy link

@thaJeztah I have no clue how breaking this will be. It really depends on if people parse the byte sizes.

This is to be closer to common Linux outputs.
Most Linux tools output with space before the human size unit.
Examples:

 - `du -h`
 - `df -h`
 - `ls -lh`
 - `tree -h`
 - `free -h`

Having a space breaks `sort -h` (and makes things like `awk {print $2}`
harder).

See moby/moby#30271
@wernight
Copy link
Contributor Author

I've updated the commit description to include more details.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@thaJeztah thaJeztah merged commit 0dadbb0 into docker:master Jan 27, 2017
@thaJeztah
Copy link
Member

@wernight can you open a PR to docker/docker to update the vendored version? (and the tests?)

@wernight
Copy link
Contributor Author

There is issue moby/moby#30271 there, what would be the PR?

@thaJeztah
Copy link
Member

The PR would be to update the commit in the vendor file;
https://github.com/docker/docker/blob/master/vendor.conf#L18

Run the vndr tool (https://github.com/LK4D4/vndr/blob/master/README.md) to update the dependencies, and update (unit) tests in the docker/docker repository where needed.

Let me know if you want to work on that, or want a maintainer to take care of it.

wernight added a commit to wernight/docker that referenced this pull request Jan 31, 2017
wernight added a commit to wernight/docker that referenced this pull request Jan 31, 2017
vdemeester added a commit to vdemeester/moby that referenced this pull request Feb 7, 2017
To include  docker/go-units#23
Fixes a unit test that fails because of it.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
andrewhsu pushed a commit to docker-archive/docker-ce that referenced this pull request May 19, 2017
To include  docker/go-units#23
Fixes a unit test that fails because of it.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Upstream-commit: 6ef0b64945a6af9666ac601021d8de99317b5207
Component: cli
thaJeztah added a commit to thaJeztah/containerd that referenced this pull request Apr 23, 2019
relevant changes:

- docker/go-units#19 make 1 second not to be plural seconds
- docker/go-units#20 Add `HumanSizeWithPrecision` function
- docker/go-units#21 change week display rule
- docker/go-units#22 Better human duration precision
- docker/go-units#23 Removes spaces before unit
- docker/go-units#27 Fix containerd#26 - RAMInBytes Bug
- docker/go-units#33 Fix handling of unlimited (-1) ulimit values
- docker/go-units#34 Revert 46 minute threshold

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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