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 support for global date formats and fixed drf date formats not supported #50

Merged
merged 12 commits into from Feb 21, 2022
Merged

Conversation

rptmat57
Copy link
Collaborator

@rptmat57 rptmat57 commented Feb 15, 2022

#49
Changes:

  • added field dictionary to check field types more easily
  • added global date format settings
  • added support for rest framework date formats

This was a bit more complicated than I originally thought because the data is already formatted by drf, and because we needed to know the field type (couldn't rely on the type since it was a str).

I realized the current version would break when using drf's DATETIME_FORMAT, DATE_FORMAT or TIME_FORMAT because parse_datetime() and parse_date() could not parse non-iso formats.

Take a look and let me know what you think.

Also what should the settings be name and where should they be?
I went with DRF_RENDERER_XLSX_ prefix in global django settings:

  • DRF_RENDERER_XLSX_DATETIME_FORMAT
  • DRF_RENDERER_XLSX_DATE_FORMAT
  • DRF_RENDERER_XLSX_TIME_FORMAT

Another option would be to stick these settings inside REST_FRAMEWORK = {} instead?

- added global date format settings
- added support for rest framework date formats
@FlipperPA
Copy link
Member

@rptmat57 @willtho89 Thanks for the contribution! Before we review and merge this, I want to raise a question that's been on my mind.

Do we want to rename this package something less... obtuse? Like drf-excel or drf-spreadsheet? We could stub out the current name, and go with something easier. Thoughts?

That way, the prefix for settings could be DRF_EXCEL_ instead of DRF_RENDERER_XLSX_.

I'm not married to the current package name and figured we should have the conversation before we create global settings, to avoid headaches for people down the road.

@rptmat57
Copy link
Collaborator Author

That's probably a good time to have that discussion.
All I can say is I think it should start with drf- 😉

Jokes aside, the current name doesn't bother me. it's pretty explicit and does what it says.
Unless there is a plan to add support for more excel formats (xls?) or more spreadsheet formats (csv, gsheet, openoffice...)

Just my 2 cents.

@willtho89
Copy link
Collaborator

willtho89 commented Feb 16, 2022

Current name also doesn't bother me, but I can see where you are coming from. Other Packages mostly use djangorestframework-{format}, so djangorestframework-xlsx. As far as I can see none of them use global settings though.

I would leave this up to you @FlipperPA

@willtho89
Copy link
Collaborator

lgtm

@rptmat57 when we decide on a name please also update README.md briefly explaining these settings

@rptmat57
Copy link
Collaborator Author

sure thing. I'll update the README

With regards to settings, what do you think about having them inside REST_FRAMEWORK?

@FlipperPA
Copy link
Member

FlipperPA commented Feb 16, 2022

Let me bounce these questions off Tom Christie and a few others, since we're playing in that ecosystem. :) Great work, my friends.

If you want to follow on Twitter: https://twitter.com/FlipperPA/status/1493772563645927427

- added custom date format
- added custom number format
- global date and number formats can also be set
- added global boolean_display option
drf_renderer_xlsx/fields.py Outdated Show resolved Hide resolved
drf_renderer_xlsx/fields.py Outdated Show resolved Hide resolved
@willtho89
Copy link
Collaborator

Awesome work @rptmat57. Using cell formatting is a great feature generally.

I'm not sure how we handle breaking changes. Imo Deprecation warnings would be best. @FlipperPA what would you suggest?

@FlipperPA
Copy link
Member

Deprecation warnings would be best - and I think we're probably around time for a 1.0 release and semver for breaking changes. I'll get us renamed to drf-spreadsheet today so that we can move forward with an easier name, and keep the old name as a stub. Thanks so much for both of your hard work on this!

@willtho89
Copy link
Collaborator

lgtm – aside from the new datetime formatting.

tested with some of our existing data/serializers. Had to rewrite the xlsx_date_format_mappings tough

@rptmat57
Copy link
Collaborator Author

would supporting both formats using a strftime_format property (or something similar) in the mapping dict be overkill?

@rptmat57
Copy link
Collaborator Author

Actually, nevermind. I would suggest that we could now use one format dictionary where both date and number formats could be defined. It would make things much easier. all following openpyxl formats

@rptmat57
Copy link
Collaborator Author

something like this:

format_mappings = {
    'created_at': 'yyyy-mm-dd h:mm:ss',
    'cost': '"$"#,##0.00_-',
}

@FlipperPA
Copy link
Member

@rptmat57 @willtho89 Renaming is complete! I ended up going with drf-excel, since it is the only format we actually support.

You should be able to git remote set-url origin git@github.com:wharton/drf-excel.git and carry on. :)

Copy link
Collaborator Author

@rptmat57 rptmat57 left a comment

Choose a reason for hiding this comment

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

should be done with changes now

@FlipperPA
Copy link
Member

@rptmat57 This is looking really impressive. Thanks for putting so much effort in!

@rptmat57
Copy link
Collaborator Author

@FlipperPA thanks! I am hoping it will make dealing with formatting easier, and it will be very useful to me so it's a win-win.
Nice little plugin you guys put together

@rptmat57
Copy link
Collaborator Author

alright I am really done now. added styling/formatting per column

@willtho89 willtho89 self-requested a review February 20, 2022 12:38
@FlipperPA
Copy link
Member

@rptmat57 @willtho89 I'm good with merging this as long as you both are. Does this break backwards compatibility? Should this become released version 2.0.0?

We'll never get SemVer perfect, I'm sure, but should do our best. :)

@rptmat57
Copy link
Collaborator Author

it does break backwards compatibility with regards to date formats.
I am sorry I was trying to get the changes in before you release 1.0.0 but didn't realized it had already been released... my bad

@FlipperPA
Copy link
Member

No biggie at all! I wanted the 1.0 release to be the name change, so 2.0 is just fine.

@FlipperPA FlipperPA merged commit 81a06b5 into wharton:main Feb 21, 2022
@LeeHanYeong
Copy link
Contributor

When will version 2.0 with code written by rptmat57 be released? I was watching this conversation because I needed the number format function.

@FlipperPA
Copy link
Member

@LeeHanYeong I'm going to get it out the door shortly. Today. :)

@rptmat57
Copy link
Collaborator Author

Nice, thanks!
I believe #4 and #30 can now be closed, since this is supported in 2.0.0

@FlipperPA
Copy link
Member

Great work @rptmat57! Thanks so much, this is a major improvement. We are live!

https://pypi.org/project/drf-excel/2.0.0/

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

4 participants