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

time parsing #276

Merged
merged 36 commits into from Apr 6, 2020
Merged

time parsing #276

merged 36 commits into from Apr 6, 2020

Conversation

izulin
Copy link
Collaborator

@izulin izulin commented Mar 27, 2020

Context

Custom time parsing.

How has this been tested?

unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. Time and DateTime types and duration #229

Checklist:

  • My code follows the code style of this project,
  • My change requires a change to the documentation,
  • I described the modification in the CHANGELOG.md file.

@izulin
Copy link
Collaborator Author

izulin commented Mar 27, 2020

@wojciechczerniak please see whether you like this approach to custom time and date

Copy link
Contributor

@wojciechczerniak wojciechczerniak left a comment

Choose a reason for hiding this comment

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

Yup, makes sense 👍

Shouldn't we implement a sample DateTime function that should return/stringify/display a dateTime or time?

src/Config.ts Show resolved Hide resolved
@@ -2,7 +2,7 @@ import {HyperFormula, LazilyTransformingAstService} from './'
import {CellContentParser} from './CellContentParser'
import {buildColumnSearchStrategy} from './ColumnSearch/ColumnSearchStrategy'
import {Config, ConfigParams} from './Config'
import {DateHelper} from './DateHelper'
import {DateHelper} from './DateTime'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we go with the full rename?

Suggested change
import {DateHelper} from './DateTime'
import {DateTimeHelper} from './DateTimeHelper'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is more to this file than the Helper. At one point we might split the file into -Helper and -Default

*/
parseDate: (dateString: string, dateFormats: string) => Maybe<SimpleDate>,
parseDateTime: (dateTimeString: string, dateFormat: string, timeFormat: string) => Maybe<DateTime>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to report this anyway, so maybe BTW we could fix it: dateFormat is not sufficient here. We don't have access to leapYear1900, nullDate, nullYear. My suggestion would be to pass the whole Config as an argument instead of listing them all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO we shouldn't have access to those options from the level of those functions. I specifically did a refactor, so that 'parseDate' is doing what it promises: parses a date format. The logic behind transforming date object into number is hidden in the engine and shouldn't be a part of parsing function.

src/Config.ts Outdated
*
* @default defaultStringifyDate
*/
stringifyDate: (date: SimpleDate, formatArg: string) => Maybe<string>,
stringifyDate: (date: SimpleDate, dateFormat: string) => Maybe<string>,
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not stringify the DateTime object?

@izulin
Copy link
Collaborator Author

izulin commented Mar 27, 2020

Yup, makes sense 👍

Shouldn't we implement a sample DateTime function that should return/stringify/display a dateTime or time?

Sorry, I meant to write that this is WIP. I implemented first half of the issue, and will proceed to the second half now.

@wojciechczerniak
Copy link
Contributor

Yup, makes sense 👍
Shouldn't we implement a sample DateTime function that should return/stringify/display a dateTime or time?

Sorry, I meant to write that this is WIP. I implemented first half of the issue, and will proceed to the second half now.

OK. Let me know when it's ready for a review

@izulin
Copy link
Collaborator Author

izulin commented Mar 30, 2020

@wojciechczerniak take a look

@izulin
Copy link
Collaborator Author

izulin commented Mar 30, 2020

the only deviation from standard formatting (that is used in e.g. moment.js) is that moment.js uses HH and hh to distinguish between 24 and 12 hour formatting. But still, moment.js defines token 'a'. We use token 'a', which is ignored during time/parsing (since we allow for am/pm modifiers always anyway, but only at the end of the time-string). For printing, if there is token 'a', then we use 12h, and 24h system otherwise.

@izulin izulin merged commit de7bbcc into develop Apr 6, 2020
@wojciechczerniak wojciechczerniak deleted the pu/issue-229 branch April 30, 2020 14:31
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

2 participants