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

Custom DateTime format #616

Merged
merged 12 commits into from
Jun 13, 2024
Merged

Conversation

DancePanda42
Copy link
Contributor

Allows the user to define a user-defined date format, whereby the date information is not lost.

this will be fix also the issue #424

sorry but I also had to remove the goto 😄

@jiaguangli
Copy link
Member

thanks for your pull request,
i have replied issure #424,
Do we really need this feature?
We should discuss this seriously.

@DancePanda42
Copy link
Contributor Author

DancePanda42 commented Jun 11, 2024

Hi,
For me, the main reason for this feature is that it makes it easier to use the filter for a date in Excel.
Which unfortunately is not the case if the date is a string.
Maybe I didn't understand the issue correctly. I thought that would pursue the same goal.

@jiaguangli
Copy link
Member

jiaguangli commented Jun 11, 2024

Hi, For me, the main reason for this feature is that it makes it easier to use the filter for a date in Excel. Which unfortunately is not the case if the date is a string. Maybe I didn't understand the issue correctly. I thought that would pursue the same goal.

No, you are correct, but we have to consider whether this approach makes sense and whether it will cause trouble in the future.
It's really hard to decide.
As a developer, this is good, but for non-developers, it may not be very practical

Please don't remove your pull request, maybe we will need this feature in the future

@jiaguangli
Copy link
Member

image
image
image

Format = "dd.mm.yyyy hh:mm:ss"
doesn't actually work for excel,Maybe this is not the result we want.

@DancePanda42
Copy link
Contributor Author

image
image

It was never my intention to change the default date format to dd.mm.yyyy hh:mm:ss.
My PR rather referred to the comment // TODO: now it'll lose date type information.
As a result, nothing should change for the users, except that the date information will now be available in Excel.
Of course, if users have been further processing the string within the Excel table, this would be a breaking change since it is now a DateOA double instead of a string.

@jiaguangli
Copy link
Member

jiaguangli commented Jun 13, 2024

Sorry,
I misunderstood what you meant before ,

Can SaveAsync() also be supported?
And GenerateSheetByIDataReader
Thank you for your contribution

@DancePanda42
Copy link
Contributor Author

Hi, yes no problem.
I think the reference to the issue took things in the wrong direction.

Support for async should not be a big effort anymore.
The support for GenerateSheetByIDataReader is already provided.
DynamicColumnsConfigurationIsUsedWhenCreatingExcelUsingIDataReader

Additionally, I was considering implementing the support for the templates as well. For that, I will open a new PR when the time comes.

Update GenerateSheetByIDataReader
Copy link
Member

@jiaguangli jiaguangli left a comment

Choose a reason for hiding this comment

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

good idea

@jiaguangli
Copy link
Member

jiaguangli commented Jun 13, 2024

Hi, yes no problem. I think the reference to the issue took things in the wrong direction.

Support for async should not be a big effort anymore. The support for GenerateSheetByIDataReader is already provided. DynamicColumnsConfigurationIsUsedWhenCreatingExcelUsingIDataReader

Additionally, I was considering implementing the support for the templates as well. For that, I will open a new PR when the time comes.

Hi,please sign your commits,Learn more about signing commits

@shps951023 shps951023 merged commit 00a445c into mini-software:master Jun 13, 2024
3 checks passed
@shps951023
Copy link
Member

👍👍👍
@DancePanda42 Could we invite you to our team?

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.

3 participants