Implement display options for Duration type #1091

silentninja · 2022-02-21T21:23:11Z

Fixes #433

Adds display options for the duration type.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

…tions' into mathesar-433-duration-display-options

kgodey

@silentninja It seems like this point from #433 is not addressed here:

(4) We should add supported display options to the types endpoint.

See individual comments also.

kgodey · 2022-02-23T00:54:16Z

mathesar/tests/api/test_column_api.py

@@ -256,6 +257,7 @@ def test_column_create_display_options(
    name = "anewcolumn"
    data = {"name": name, "type": type_, "display_options": display_options}
    response = client.post(f"/api/db/v0/tables/{column_test_table.id}/columns/", data)
+    print(response.data)


Please remove this print statement.

kgodey · 2022-02-23T01:00:25Z

mathesar/api/serializers/shared_serializers.py

@@ -180,6 +180,14 @@ def validate(self, datetime_obj, display_format, serializer_field):
        return super().validate(datetime_obj, display_format, serializer_field)


+class DurationFormatValidator(TimeWithoutTimeZoneFormatValidator):
+    """
+    The validation is same as Time Without TimeZone as of now,


This does not make sense. Durations can span weeks, months, days, and years. Times cannot.

Fixed it, it was meant to be timestamp. I have changed the validation method anyway since we need a different error message, though the logic is similar.

Also, I haven't restricted using a weekdate format string(2019-W17), though the duration value returned from the database follows RFC 3339 and does not return a weekdate, as the user might have a different requirement

kgodey · 2022-02-23T01:02:40Z

I also wanted to verify that this point was addressed, I don't see it anywhere.

(3) If the column type is changed, the display options should be deleted.

codecov-commenter · 2022-02-23T04:08:31Z

Codecov Report

Merging #1091 (e124ca2) into master (8591a14) will decrease coverage by 0.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
- Coverage   93.25%   93.24%   -0.02%     
==========================================
  Files         112      112              
  Lines        4078     4084       +6     
==========================================
+ Hits         3803     3808       +5     
- Misses        275      276       +1

Flag	Coverage Δ
pytest-backend	`93.24% <85.71%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
mathesar/api/display_options.py	`100.00% <ø> (ø)`
mathesar/api/serializers/shared_serializers.py	`86.04% <85.71%> (-0.14%)`	⬇️

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 8591a14...e124ca2. Read the comment docs.

…ead of time

…tions' into mathesar-433-duration-display-options

silentninja · 2022-02-23T04:53:50Z

I also wanted to verify that this point was addressed, I don't see it anywhere.

(3) If the column type is changed, the display options should be deleted.

This was handled as part of the initial display options PR, it is taken care of when a column is reflected

silentninja · 2022-02-23T04:57:47Z

(4) We should add supported display options to the types endpoint.

Available Display options are part of the types serializer. I have added it to the list

kgodey · 2022-02-23T05:59:47Z

Available Display options are part of the types serializer. I have added it to the list

Sorry I missed this, I think I reviewed too many PRs in a row. :/

mathesar-433 Implement display options for Duration type

766f24e

silentninja requested a review from a team February 21, 2022 21:23

Merge branch 'master' into mathesar-433-duration-display-options

74f2c40

github-actions bot requested review from dmos62, eito-fis, kgodey, mathemancer and pavish February 21, 2022 21:23

silentninja added 2 commits February 22, 2022 22:04

Add Duration type display options to types list

683847d

Merge remote-tracking branch 'origin/mathesar-433-duration-display-op…

6c22eaa

…tions' into mathesar-433-duration-display-options

kgodey self-assigned this Feb 22, 2022

kgodey added the pr-status: review A PR awaiting review label Feb 22, 2022

kgodey requested changes Feb 23, 2022

View reviewed changes

kgodey added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Feb 23, 2022

kgodey changed the title ~~mathesar-433 Implement display options for Duration type~~ Implement display options for Duration type Feb 23, 2022

kgodey assigned silentninja and unassigned kgodey Feb 23, 2022

Merge branch 'master' into mathesar-433-duration-display-options

4a9f1e0

silentninja added 2 commits February 23, 2022 09:50

Fix duration display options to accept timestamp display options inst…

f7692f3

…ead of time

Merge remote-tracking branch 'origin/mathesar-433-duration-display-op…

9b7acc8

…tions' into mathesar-433-duration-display-options

silentninja requested a review from kgodey February 24, 2022 23:06

silentninja assigned kgodey and unassigned silentninja Feb 24, 2022

Merge branch 'master' into mathesar-433-duration-display-options

e124ca2

kgodey added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Feb 24, 2022

kgodey approved these changes Feb 25, 2022

View reviewed changes

kgodey merged commit 02e22cb into master Feb 25, 2022

kgodey deleted the mathesar-433-duration-display-options branch February 25, 2022 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement display options for Duration type #1091

Implement display options for Duration type #1091

silentninja commented Feb 21, 2022 •

edited

Loading

kgodey left a comment

kgodey Feb 23, 2022

kgodey Feb 23, 2022

silentninja Feb 23, 2022

silentninja Feb 23, 2022

kgodey commented Feb 23, 2022

codecov-commenter commented Feb 23, 2022 •

edited

Loading

silentninja commented Feb 23, 2022

silentninja commented Feb 23, 2022 •

edited

Loading

kgodey commented Feb 23, 2022

Implement display options for Duration type #1091

Implement display options for Duration type #1091

Conversation

silentninja commented Feb 21, 2022 • edited Loading

Checklist

Developer Certificate of Origin

kgodey left a comment

Choose a reason for hiding this comment

kgodey Feb 23, 2022

Choose a reason for hiding this comment

kgodey Feb 23, 2022

Choose a reason for hiding this comment

silentninja Feb 23, 2022

Choose a reason for hiding this comment

silentninja Feb 23, 2022

Choose a reason for hiding this comment

kgodey commented Feb 23, 2022

codecov-commenter commented Feb 23, 2022 • edited Loading

Codecov Report

silentninja commented Feb 23, 2022

silentninja commented Feb 23, 2022 • edited Loading

kgodey commented Feb 23, 2022

silentninja commented Feb 21, 2022 •

edited

Loading

codecov-commenter commented Feb 23, 2022 •

edited

Loading

silentninja commented Feb 23, 2022 •

edited

Loading