initial work to use date type objects for the date and not strings #350
Conversation
|
Hi @kinkerl Sorry for the delay. Can you update with the latest changes and fix the conflict ? Thanks! I think the feature looks good... as a workaround I think that you can use a custom I think that jinja defaults to >>> from datetime import date
>>> ca = date.today()
>>> str(ca)
'2022-02-13'
>>> ca.isoformat()
'2022-02-13'
>>> ca.strftime("%B")Now... if someone uses some strange "build --date=Q1-2022" format.. we will break that build...but I think this is ok as I hope people have normal dates :) >>> parse("Q1-2022")
dateutil.parser._parser.ParserError: Unknown string format: Q1-2022I few questions/comments for this:
|
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
|
hey @adiroiban I made the changes and added first tests. The tests could be adapted once I figure out how to actually run them properly locally. I am not happy about the required template changes. Any input (see my comments)? |
There was a problem hiding this comment.
Thanks for the update and for moving this forward.
This looks good...but I have a few comments regarding the extend of these changes.
It is not clear why the default template needs to be changed.
The change might be needed and there might a good reason behind that change. but i don't see any info about this in the description of this PR.
Also, I think that we should focus the test for datetime to only check the datetime rendering and don't worry about the other rendered objects... we have plenty of tests for the full template rendering.
| @@ -0,0 +1 @@ | |||
| The `versiondata.date` is not `date` instance. This allows users to customize the date rendering in the template with `strftime`, like so: `{{versiondata.date.strftime("%B")}}`. | |||
There was a problem hiding this comment.
| The `versiondata.date` is not `date` instance. This allows users to customize the date rendering in the template with `strftime`, like so: `{{versiondata.date.strftime("%B")}}`. | |
| The `versiondata.date` is now a `date` instance. | |
| This allows users to customize the date rendering in the template with `strftime`. | |
| For example by using `{{versiondata.date.strftime("%B")}}`. |
| @@ -3,10 +3,10 @@ | |||
| {{ top_underline * ((top_line)|length)}} | |||
| {% elif versiondata.name %} | |||
| {{ versiondata.name }} {{ versiondata.version }} ({{ versiondata.date }}) | |||
| {{ top_underline * ((versiondata.name + versiondata.version + versiondata.date)|length + 4)}} | |||
| {{ top_underline * ((versiondata.name + versiondata.version)|length + 4)}}{{ top_underline * (versiondata.date|title|length)}} | |||
There was a problem hiding this comment.
Why do we need these changes to the default template?
I was thinking that the scope and purpose of the PR is to just change the internal of the date object.
| fragments = split_fragments(fragments={}, definitions=definitions) | ||
|
|
||
| template = pkg_resources.resource_string( | ||
| "towncrier", "test/templates/date.rst" |
There was a problem hiding this comment.
I think that this file should hint the actual test in which it is used.
| "towncrier", "test/templates/date.rst" | |
| "towncrier", "test/templates/test_write_test_dateformat.rst" |
but I hope that we can have a very simple and minimal template
something like
template = """
We have a full datetime object: versiondata.date|date:"D d M Y" }
""".strip()
And check the expected output.
src/towncrier/test/templates/date.rst has some many side-effects and things that are not related to version datetime functionaliyt that is hard to see what is the target of the test.
|
It looks like the automated tests are failing. We need a green CI run before this can be merged. I hope that by reverting the default template format and simplifying the new test, this PR will be green. Thanks! |
|
@kinkerl Just checking in on this issue. Do you have plans to revisit the concerns above? |
|
i totally lost track of this but i can check them out again next week! |
| project_date = _get_date().strip() | ||
| project_date = date.today() | ||
| else: | ||
| project_date = parse(project_date).date() |
There was a problem hiding this comment.
Just a minor comment
Can we just support ISO date format and then use stdlib datetime.strptime to parse the date ?
Not a big deal... just a shame that we dont't have "smart" parse function in stdlib.
At the same time, I think that we should encourage standard date representation, and only support ISO format.
... and it this way, we don't need to add yet another dependency
Hi,
this is my first change to support better date objects during template rendering.
This PR aims to solve #349.
Cheers