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

Implement date functions #85

Merged
merged 9 commits into from
Jul 5, 2023
Merged

Implement date functions #85

merged 9 commits into from
Jul 5, 2023

Conversation

baodrate
Copy link
Contributor

Implement date functions:

  • now
  • fromdateiso8601
  • todateiso8601

@baodrate
Copy link
Contributor Author

todateiso8601 produces different formats depending on the input type:

  • YYYY-MM-DDThh:mm:ss±hh:mm for ints
  • YYYY-MM-DDThh:mm:ss.sssssssss±hh:mm for floats

I wanted to do the similar for fromdateiso8601 but wasn't happy with how bloated it made the function. Implementation suggestions would be appreciated

The other date functions (i.e. strftime/strptime) don't have equivalents in the time crate. Could maybe use https://github.com/MiSawa/time-fmt/

@01mf02
Copy link
Owner

01mf02 commented Jun 26, 2023

I wanted to do the similar for fromdateiso8601 but wasn't happy with how bloated it made the function. Implementation suggestions would be appreciated

To help you with this, would you like to elaborate how you were trying to approach the problem?

The other date functions (i.e. strftime/strptime) don't have equivalents in the time crate. Could maybe use https://github.com/MiSawa/time-fmt/

I'd say let's go for now only for the ISO format, and postpone the hard case if there is sufficient demand later. :)

@01mf02 01mf02 mentioned this pull request Jun 28, 2023
@baodrate
Copy link
Contributor Author

To help you with this, would you like to elaborate how you were trying to approach the problem?

I don't remember exactly what I did, but it was pretty involved. Looking at it with fresh eyes, the problem seems a lot simpler. 😅 I added the new implementation in 7bc1b54. tell me what you think

@baodrate
Copy link
Contributor Author

Looks like tests are failing because I added time as an optional dependency without properly feature-gating all the code, but I got a bit stuck on the best way to conditionally chain the datetime filters to the core filters:

jaq/jaq-core/src/core.rs

Lines 12 to 21 in 5184281

pub fn core() -> impl Iterator<Item = (String, usize, Native)> {
// TODO: make this more compact
let core_run = CORE_RUN
.iter()
.map(|(name, arity, f)| (name.to_string(), *arity, Native::new(*f)));
let core_update = CORE_UPDATE.iter().map(|(name, arity, run, update)| {
(name.to_string(), *arity, Native::with_update(*run, *update))
});
core_run.chain(core_update)
}

Should I just add time a non-optional dependency? Or should I go ahead and move all the code that depends on it behind feature-gates?

@01mf02
Copy link
Owner

01mf02 commented Jul 4, 2023

You can go ahead and make time a non-optional dependency.
(For the same reason why regex is a non-optional dependency: I find it confusing if the set of available commands depends on features.)

@01mf02
Copy link
Owner

01mf02 commented Jul 4, 2023

I don't remember exactly what I did, but it was pretty involved. Looking at it with fresh eyes, the problem seems a lot simpler. sweat_smile I added the new implementation in 7bc1b54. tell me what you think

That looks nice now that the functions round-trip.

@01mf02 01mf02 mentioned this pull request Jul 4, 2023
@baodrate
Copy link
Contributor Author

baodrate commented Jul 5, 2023

Just now realized that SystemTime requires std, which means now is a no-go. How do you think this case should be handled?

@01mf02 01mf02 merged commit 1c35c5d into 01mf02:main Jul 5, 2023
1 check failed
@01mf02
Copy link
Owner

01mf02 commented Jul 5, 2023

Don't worry about std. I have a plan anyway to change the core filter implementation, and this will address the std issue.

@baodrate baodrate deleted the date-funcs branch July 7, 2023 10:14
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