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

fix: allow default values for input arguments #1939

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ssddOnTop
Copy link
Member

Issue Reference(s):
Fixes #1931
/claim 1931

@github-actions github-actions bot added the type: fix Iterations on existing features or infrastructure. label May 13, 2024
@ssddOnTop ssddOnTop marked this pull request as ready for review May 14, 2024 08:10
Copy link

codecov bot commented May 14, 2024

Codecov Report

Attention: Patch coverage is 93.10345% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 82.10%. Comparing base (7f9913d) to head (c6aaf67).
Report is 16 commits behind head on main.

Current head c6aaf67 differs from pull request most recent head d0ce322

Please upload reports for the commit d0ce322 to get more accurate results.

Files Patch % Lines
src/core/blueprint/into_schema.rs 93.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1939      +/-   ##
==========================================
+ Coverage   82.08%   82.10%   +0.02%     
==========================================
  Files         173      173              
  Lines       17623    17644      +21     
==========================================
+ Hits        14465    14486      +21     
  Misses       3158     3158              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tusharmath tusharmath added the ci: benchmark Runs benchmarks label May 14, 2024
Copy link

github-actions bot commented May 14, 2024

🐰Bencher

ReportSat, May 18, 2024 at 03:45:12 UTC
Projecttailcall
Branch1939/merge
Testbedbenchmarking-runner

🚨 1 ALERT: Threshold Boundary Limit exceeded!
BenchmarkMeasure (units)ViewValueLower BoundaryUpper Boundary
input/vars.missingLatency (nanoseconds (ns))🚨 (view plot | view alert)11.73 (+37.26%)10.95 (107.14%)

Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
group_by✅ (view plot)535.10 (-10.68%)648.29 (82.54%)
input/args.missing✅ (view plot)22.93 (-7.37%)27.22 (84.23%)
input/args.nested.existing✅ (view plot)42.66 (-18.05%)65.96 (64.67%)
input/args.nested.missing✅ (view plot)37.38 (-3.00%)40.63 (92.01%)
input/args.root✅ (view plot)37.92 (-21.97%)62.51 (60.67%)
input/headers.existing✅ (view plot)33.03 (+3.91%)33.50 (98.61%)
input/headers.missing✅ (view plot)31.55 (+2.47%)33.37 (94.52%)
input/value.missing✅ (view plot)23.19 (-1.54%)25.36 (91.41%)
input/value.nested.existing✅ (view plot)42.07 (+1.40%)43.95 (95.72%)
input/value.nested.missing✅ (view plot)36.84 (+1.19%)38.34 (96.09%)
input/value.root✅ (view plot)37.57 (-2.40%)40.38 (93.04%)
input/vars.existing✅ (view plot)7.44 (-8.94%)9.06 (82.05%)
input/vars.missing🚨 (view plot | view alert)11.73 (+37.26%)10.95 (107.14%)
test_batched_body✅ (view plot)2,682.70 (-99.69%)2,654,750.22 (0.10%)
test_batched_body #2✅ (view plot)1,634,100.00 (-6.06%)1,888,131.04 (86.55%)
test_data_loader✅ (view plot)460,710.00 (-2.46%)490,377.11 (93.95%)
test_handle_request✅ (view plot)157,400.00 (-7.82%)182,978.09 (86.02%)
with_mustache_expressions✅ (view plot)1,183.80 (+1.04%)1,236.24 (95.76%)
with_mustache_literal✅ (view plot)741.64 (+2.43%)772.57 (96.00%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

} else {
async_graphql_value::to_value(args).unwrap_or_default()
};

let mut dyn_schema_field = dynamic::Field::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just call the the dyn_schema_field.arguments(args) Current implementation is a bit complicated and unnecessary.

Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 17, 2024
@ssddOnTop ssddOnTop marked this pull request as draft May 18, 2024 03:30
@tusharmath tusharmath removed the ci: benchmark Runs benchmarks label May 18, 2024
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 20, 2024
@ssddOnTop ssddOnTop removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 24, 2024
@ssddOnTop
Copy link
Member Author

there is an issue with async-graphql as well, I raised a PR async-graphql/async-graphql#1527 there if it gets merged, I'll finish touchups in this PR.

Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim type: fix Iterations on existing features or infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: allow default values for input arguments.
2 participants