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

memory unit formatter #131

Merged
merged 2 commits into from Oct 7, 2017

Conversation

Projects
None yet
2 participants
@elpikel
Contributor

elpikel commented Oct 6, 2017

@PragTob

Thanks for the PR! I got a couple of minor points, mostly related to formatting but otherwise should be good to go :)

Nala, Al and Ghost welcome your efforts!

img_20171003_101022

@bytes_per_terabyte @bytes_per_gigabyte * @bytes_per_kilobyte
@units %{
terabyte: %Unit{

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

formatting is a bit curious here, e.g. no other unit seems to be so big to need to indent that far.

@units %{
terabyte: %Unit{
name: :terabyte,

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

Plus it seems the things within the unit aren't indented, I prefer it more like this:

%{
  longest_unit: %Unit{
                  property1: val1,
                  prop2:     val2
                }
}

that's also what it hopefully should be like in the other formatters :)

This comment has been minimized.

@elpikel

elpikel Oct 7, 2017

Contributor

is there any tool that could do that for me?

iex> value
1.205078125
iex> unit.name
:kilobyte

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

Yay, doctests!

unit. In case of tie, chooses the largest of the most common units.
Pass `[strategy: :smallest]` to always return the smallest unit in the list.
Pass `[strategy: :largest]` to always return the largest unit in the list.

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

I think we support 4 strategies, weird to just mention 2. But that might be duplicated from other places anyhow...

This comment has been minimized.

@elpikel

elpikel Oct 7, 2017

Contributor

good point

end
@doc """
The most basic unit in which momory occur, byte.

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

I think this should be memory :)

defp format_memory(memory, coefficient), do: "#{Float.round(memory / coefficient, 2)} GB"
defp format_memory(memory) do
scaled_memory = Memory.scale(memory, :gigabyte)
Memory.format({scaled_memory, :gigabyte})

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

I think just calling Memory.format(memory) should give us the right result, it mgiht not always be GB as before but if it decides not to do GB then I think that's what we want :)

This comment has been minimized.

@elpikel

elpikel Oct 7, 2017

Contributor

i did not want to change how it was implemented before... and i missed the point of implementing memory formatter :)

end
defp parse_memory_for(:Linux, raw_output) do
["MemTotal:" <> memory] = Regex.run(~r/MemTotal.*kB/, raw_output)
{memory, _} = memory
|> String.trim()
|> String.trim_trailing(" kB")
|> Integer.parse
format_memory(memory, @kilobyte_to_gigabyte)
format_memory(memory * 1024)

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

This impromptu conversion isn't ideal imo. I think it'd be better if we explicitly took it as kilobytes and converted it to bytes and then passed it into th format_memory function

describe ".format" do
test ".format(1_023)" do
assert format(1_023) == "1023 B"

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

indentation seems off here )

This comment has been minimized.

@PragTob

PragTob Oct 7, 2017

Owner

but good edge case testing!

@PragTob

Looks good, there is still that 1024 in non memory code floating around that I want to get rid off. Not 100% sure which method to call for that myself, so I'll go ahead and add it on top myself and then merge it :)

Thanks!

@PragTob

This comment has been minimized.

Owner

PragTob commented Oct 7, 2017

Shockingly we don't seem to have functions yet to scale units down or convert from one unit to the other 😱

That can be done in a separate ticket though :)

@PragTob PragTob merged commit 83ddcee into PragTob:master Oct 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 94.587%
Details

This was referenced Oct 7, 2017

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