f-string formatting

[georgpfolz]

Fixes #35

added __format__ method to DateTime: enables formatting of the date in f-strings and with String format() (like in datetime):

from DateTime import DateTime
d = DateTime()
print(f'my formatted date: {d:%d.%m.%Y}')
print('my formatted date: {d:%d.%m.%Y}'.format(d))

[dataflake]

Once the failing tests are fixed I'll take a look

[d-maurer]

Georg Pfolz wrote at 2022-6-26 23:36 -0700:

...

-- Commit Summary --

• f-string formatting

-- File Changes --

M src/DateTime/DateTime.py (8) M src/DateTime/tests/test_datetime.py (6)

Please add an entry in CHANGES.rst.

[georgpfolz]

Please add an entry in CHANGES.rst. ok!

As I can see, there is already an empty entry for version 4.5. So I guess I'll put the changes in there.

I'll leave the version number in setup.py unchanged, ok?

(Sorry I don't have a lot of experience with PRs)

[dataflake]

Things like version numbers in setup.py are changed only by package managers who make releases etc. Developer pull requests should only contain code changes, tests for these changes and a change log entry so people know what has changed from one version to the next.

[dataflake]

Don't forget that it's your job to fix the test failures before a code review can take place.

[dataflake]

FYI: f-strings are only available on Python 3.6 and higher. The __format__ and format methods for strings exists on Python 3.5. So you should skip the test on Python 2.7 and pypy (which is equivalent to Python 2.7) and use the explicit format method instead of an f-string in the test.

[georgpfolz]

Don't forget that it's your job to fix the test failures before a code review can take place.

ok 😓

I'm nearly done (testing on my github repo for DateTime), but two errors remain and I don't know how to fix them:

(I'm using using pip 22.1.2 locally)

coverage:

ERROR: InvocationError for command /home/runner/work/DateTime/DateTime/.tox/coverage/bin/coverage report -m --fail-under=88 (exited with code 2)

[jugmac00]

@georgpfolz Using an older pip version is no issue - the build succeeded, although annoyingly the following message is printed:

Error: You are using pip version 20.3.4, however version 22.1.2 is available.

I have not seen this before - imho this should be a warning, not an error.

When I have a look at your builds, I identify the following issues:

a) pypy is broken, see https://github.com/zopefoundation/DateTime/runs/7068251649?check_suite_focus=true

Test-module import failures:
Module: DateTime.tests.test_datetime
  File "/home/runner/work/DateTime/DateTime/src/DateTime/tests/test_datetime.py", line 690
    self.assertEqual(result, f'{dt:%-d.%-m.%Y %H:%M}')
                              ^
SyntaxError: invalid syntax (expected ')')

I can't help with that.

b) Coverage dropped...

https://github.com/zopefoundation/DateTime/runs/7068251796?check_suite_focus=true

TOTAL                                  1540    148    328     59    87.96%
Coverage failure: total of 87.96 is less than fail-under=88.00
ERROR: InvocationError for command /home/runner/work/DateTime/DateTime/.tox/coverage/bin/coverage report -m --fail-under=88 (exited with code 2)
___________________________________ summary ____________________________________
ERROR:   coverage: commands failed
Error: Process completed with exit code 1.

This may mean that you added code which is not tested, maybe only a branch which is not tested.

I would run tox -e coverage on the local developer machine, and have a look at the generated html coverage report, especially at the code you added.

[georgpfolz]

@jugmac00

Thank you for the quick answer!

I already fixed the pypy problem (not pushed to zopefoundation yet).

In the meantime if figured out about coverage: I need to insert a # pragma commentary to the condition where I exclude the older Python versions (< 2.7).

The pip warning is strange, it only happens in the py35 test, and it did not happen in one of the earlier tests. I'll look into this before pushing.

[dataflake]

The pip warning is strange, it only happens in the py35 test, and it did not happen in one of the earlier tests. I'll look into this before pushing.

Do not spend time on that warning, it simply doesn't matter.

[georgpfolz]

I finally got rid of the coverage-related errors: the last one wasn't even in my code:

try:
    __file__
except NameError:  # pragma: no cover
    f = sys.argv[0]
else:
    f = __file__

Before I push it to zopefoundation, one last question: Because I used GitHub to do the tests, I have a lot of commits in this branch. Is that ok for merging or should I rebase the last commits into one?

@dataflake

Do not spend time on that warning, it simply doesn't matter.

OK!

[d-maurer]

Georg Pfolz wrote at 2022-6-27 04:25 -0700:

OK, I got rid of the coverage-related error: the last one wasn't even in my code:

try:
   __file__
except NameError:  # pragma: no cover
   f = sys.argv[0]
else:
   f = __file__

Before I push it to zopefoundation, one last question: Because I used GitHub to do the tests, I have a lot of commits in this branch. Is that ok for merging or should I rebase the last commits into one?

The number of commits does not pose a problem.

The merges into master will by default combine all PR commits into a single one.

[georgpfolz]

The merges into master will by default combine all PR commits into a single one.

Good to know. Pushed.

Thank you everyone for your guidance and your patience! :)

[jugmac00]

@georgpfolz FWIW: The pip "error" is actually a bug in the "python-versions" GitHub action, which has been fixed for Python 3.8 and higher.

https://github.com/actions/python-versions/pull/117

[dataflake]

@georgpfolz I have made some small changes to the change log entry so it refers to the specific GitHub issue, and I changed how and when the Python version is checked during unit testing. Now the entire test will be skipped under Python 2.7/pypy, and I re-used an existing definition for Python 3 higher up in the file. No need to re-invent the wheel.

What you normally do when your PR is ready for review, meaning once the unit tests succeed, is to assign reviewers. When I am unfamiliar with who the package developers are I look at the list of recent commits and pick people from there. Once you get approvals from the review you click on the merge button yourself. You will notice it's unavailable right now - the review approvals need to come first.

[georgpfolz]

@dataflake

What you normally do when your PR is ready for review, meaning once the unit tests succeed, is to assign reviewers. When I am unfamiliar with who the package developers are I look at the list of recent commits and pick people from there. Once you get approvals from the review you click on the merge button yourself. You will notice it's unavailable right now - the review approvals need to come first.

Ok, I assigned some reviewers.

Learned a lot today! :)

[georgpfolz]

While doing the requested changes, I tried readding the tests for the actual f-strings (the reasoning being that otherwise the tests would pass even without the __format__ method I added to DateTime).

But the PyPy test consistently failed. I tried to exclude the f-string check with the IS_PYPY constant in the test file, and also with not hasattr(sys, 'pypy_version_info') and platform.python_implementation() != 'PyPy' and platform.python_implementation() == 'CPython'.

This doesn't seem to have any influence on the test, the code in the condition block gets executed anyway during the pypy test.

Is there a way to do this, should I do this, or should I just remove the code and forget about it?

(I'm doing these tests on the repository on my account, I haven't pushed anything yet to the zopefoundation)

[d-maurer]

Georg Pfolz wrote at 2022-6-28 05:28 -0700:

... Is there a way to check for pypy, or should I just not test the f-strings?

That pypy does not work is due to the fact that it is Python 2.7 (f-strings are only available in Python 3). You can check for the Python version via a sys attribute or (if DateTime already depends in six) a standard six definition.

[georgpfolz]

That pypy does not work is due to the fact that it is Python 2.7 (f-strings are only available in Python 3). You can check for the Python version via a sys attribute or (if DateTime already depends in six) a standard six definition.

I already check for sys.version_info >= (3, 6, 0), but the code is executed nonetheless in the PyPy test (I have no other version-related errors, so the check seems to work for CPython) .

(What's a six definition?)

My last try is creating a separate method for the f-string test, I used the decorator as I found it on the testStrftimeUnicode method:

@unittest.skipIf(
    IS_PYPY,
    "no f-strings in PyPy"
)
def test_format_fstring(self):
    if sys.version_info > (3, 6, 0):  # pragma: no cover
        dt = DateTime(1968, 3, 10, 23, 45, 0, 'GMT+1')
        result = '10.3.1968 22:45'
        result_with_empty_fmt = '1968/03/10 23:45:00 GMT+1'
        self.assertEqual(result, f'{dt:%-d.%-m.%Y %H:%M}')
        self.assertEqual(result_with_empty_fmt, f'{dt:}'.format(dt))
        self.assertEqual(result_with_empty_fmt, f'{dt}'.format(dt))
    else:   # pragma: no cover
        pass

But the code is still executed in the pypy test. The error returned ist the first assertion.

[dataflake]

The code is not executed. The failure happens earlier when the module is parsed. So it doesn't help trying to check for the Python version, the test module will always fail to import.

[georgpfolz]

The code is not executed. The failure happens earlier when the module is parsed. So it doesn't help trying to check for the Python version, the test module will always fail to import.

Ah, that makes sense!

Thank you, that helped me resolve the problem!

[icemac]

I just released a new version including this PR as https://pypi.org/project/DateTime/4.5/.