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

Added bsize module #4

Merged
merged 1 commit into from
Jul 17, 2018
Merged

Conversation

japokorn
Copy link
Contributor

Added module util for basic work with byte sizes
Added bsize module util test
Added bsize module

self.assertEqual(Size("1.048576e+06B").get(), "1.0 MiB")

# accept units case insensitive, without space, convert
self.assertEqual(Size("1000kilObytes").get("autodec", "%d"), "1")
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't there be a unit on the return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in this case. The second parameter of Size.get specifies the format of the value and units. %d asks only for value in int format. Units are added when %sb or %lb is used.

Copy link
Owner

Choose a reason for hiding this comment

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

I see. Thanks.

Copy link
Owner

@dwlehman dwlehman left a comment

Choose a reason for hiding this comment

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

I'm pretty sure module_utils is in the ansible core codebase and is only for functionality that could be expected to be used by the average module developer. This bsize module is only useful to the bsize module, so I think it should go into a file named something likesize.py instead.

library/bsize.py Outdated

# use whatever logic you need to determine whether or not this module
# made any modifications to your target
result['changed'] = True
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be False since it is only gathering information and not changing anything on the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

"units" parameter allows to select preferred unit.
accepted values are also:
'autobin' (default) - uses the highest human readable unit (binary)
'autodec' - uses the highest human readable unit (decimal)
Copy link
Owner

Choose a reason for hiding this comment

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

Since there are only two choices here I would suggest a boolean flag 'decimal' that defaults to None or False. It eliminates the free-form string and simplifies the method signature. I'd also make that flag the last argument.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, maybe name it something else since there is a decimal module in the standard library of python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually there are more than two possible values. "units" parameter can be also set to specific units to which the value is converted. e.g. "kilobytes" or "MiB". Updated the comment to make it clearer.

self.assertEqual(Size("1.048576e+06B").get(), "1.0 MiB")

# accept units case insensitive, without space, convert
self.assertEqual(Size("1000kilObytes").get("autodec", "%d"), "1")
Copy link
Owner

Choose a reason for hiding this comment

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

I see. Thanks.

Added module util for basic work with byte sizes
Added bsize module util test
Added bsize module
@dwlehman
Copy link
Owner

dwlehman commented Jul 9, 2018

Reference for a bundled module_utils:

https://docs.ansible.com/ansible/latest/user_guide/playbooks_best_practices.html

(See the "Directory Layout" section.)

@dwlehman dwlehman merged commit beee722 into dwlehman:master Jul 17, 2018
@dwlehman dwlehman mentioned this pull request Jul 25, 2018
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.

None yet

2 participants