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

Add support for duration primitive type in code generation #33

Merged
merged 8 commits into from
May 24, 2024

Conversation

gtalha07
Copy link
Contributor

@gtalha07 gtalha07 commented May 7, 2024

  • Implement render_literal function for Duration literals.
  • Add impl_code_type_for_primitive! and impl_renderable_for_primitive! instances for DurationCodeType.
  • Add liftDuration and lowerDuration functions to generate_wrapper_lifters and generate_wrapper_lowerers respectively.
  • Update generate_wrapper_lifters and generate_wrapper_lowerers to handle Duration type.
  • Update tests.

@gtalha07 gtalha07 linked an issue May 7, 2024 that may be closed by this pull request
@gtalha07 gtalha07 requested review from gnunicorn and chavic May 8, 2024 11:53
@gtalha07 gtalha07 marked this pull request as ready for review May 8, 2024 11:53
Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

looking good. But how did we land on adding tests for this?

@gtalha07
Copy link
Contributor Author

looking good. But how did we land on adding tests for this?

You mean these tests aren't necessary or enough for checking duration representation ? Should I remove this ?

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

sorry, missed the test files somehow. all good.

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

ehem... better to actually read the code before approving ... the tests are not testing the duration type but u64 types...

fixtures/duration_type_test/src/lib.rs Outdated Show resolved Hide resolved
@gnunicorn gnunicorn marked this pull request as draft May 15, 2024 18:25
@gnunicorn gnunicorn self-assigned this May 20, 2024
@gnunicorn gnunicorn self-requested a review May 20, 2024 15:07
@gnunicorn gnunicorn marked this pull request as ready for review May 20, 2024 15:07
@@ -365,6 +365,7 @@ pub fn generate_type(ty: &Type) -> dart::Tokens {
| Type::Int64
| Type::UInt16
| Type::Int32
| Type::Duration
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this is needed at all....

@@ -22,7 +24,8 @@ fn render_literal(literal: &Literal) -> String {
| Type::UInt32
| Type::UInt64
| Type::Float32
| Type::Float64 => num_str,
| Type::Float64
| Type::Duration => num_str,
Copy link
Contributor

Choose a reason for hiding this comment

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

neither that this is actually needed.

Copy link
Contributor

@chavic chavic left a comment

Choose a reason for hiding this comment

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

This looks good, and it looks like the tests passed

@gnunicorn gnunicorn merged commit 434d2c7 into main May 24, 2024
6 checks passed
@gnunicorn gnunicorn deleted the issue26_duration-type-support branch May 24, 2024 09:27
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.

Duration Type Support
3 participants