Skip to content

Conversation

@rsarm
Copy link
Contributor

@rsarm rsarm commented Jan 27, 2020

The current format to create TimerField objects is with a tuple (hours, minutes, seconds).
The PR changes the format to a string like '1d10h30m2s', where 'd', 'h', 'm' and 's' stand for days, hours, minutes and seconds. Simpler strings can be used as '1d' or '1h30m'.

Closes #1106

@rsarm rsarm added this to the ReFrame sprint 20.01 milestone Jan 27, 2020
@rsarm rsarm requested review from ekouts, teojgo and vkarak January 27, 2020 14:13
@rsarm rsarm self-assigned this Jan 27, 2020
@pep8speaks
Copy link

pep8speaks commented Jan 27, 2020

Hello @rsarm, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2020-02-21 12:28:30 UTC

@rsarm rsarm changed the title Change timer to a string-based format [wip] [feat] Change timer to a string-based format Jan 27, 2020
@rsarm rsarm changed the title [wip] [feat] Change timer to a string-based format [feat] Change timer to a string-based format Jan 27, 2020
@codecov-io
Copy link

codecov-io commented Jan 27, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@1a576b1). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1142   +/-   ##
=========================================
  Coverage          ?   91.78%           
=========================================
  Files             ?       81           
  Lines             ?    11391           
  Branches          ?        0           
=========================================
  Hits              ?    10455           
  Misses            ?      936           
  Partials          ?        0
Impacted Files Coverage Δ
reframe/core/containers.py 100% <100%> (ø)
unittests/test_containers.py 96.72% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a576b1...acba344. Read the comment docs.

@vkarak vkarak changed the title [feat] Change timer to a string-based format [feat] Change time_limit format to a string-based format Jan 28, 2020
@vkarak vkarak changed the title [feat] Change time_limit format to a string-based format [feat] Change time_limit attribute to a string-based format Jan 28, 2020
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

The old syntax should not be abolished, but deprecated instead. You will need also a unit test to check that a deprecation warning is raised. If we merge this as it is now, it will break several tests.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Just only some final minor suggestions.

@vkarak
Copy link
Contributor

vkarak commented Feb 20, 2020

@rsarm You'd need also to fix the conflicts before we merge.

@vkarak vkarak merged commit 1b57bc5 into reframe-hpc:master Feb 21, 2020
@rsarm rsarm deleted the feat/time-format branch March 10, 2021 08:38
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.

Follow a more intuitive time duration format for the time_limit field in RegressionTest

5 participants