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

chore: dropped pub fields from config module #2261

Closed
wants to merge 3 commits into from

Conversation

Rutik7066
Copy link
Contributor

@Rutik7066 Rutik7066 commented Jun 23, 2024

Summary:
chore: dropped pub fields from config module

Issue Reference(s):
Fixes #2256
/claim #2256
Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@Rutik7066 Rutik7066 changed the title Init chore: dropped pub fields from config module Jun 23, 2024
Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.

Project coverage is 85.70%. Comparing base (f1d8feb) to head (05cf779).

Files Patch % Lines
src/cli/tc.rs 0.00% 6 Missing ⚠️
tailcall-aws-lambda/src/main.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2261      +/-   ##
==========================================
- Coverage   85.71%   85.70%   -0.01%     
==========================================
  Files         219      219              
  Lines       21037    21047      +10     
==========================================
+ Hits        18031    18039       +8     
- Misses       3006     3008       +2     

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

Copy link

github-actions bot commented Jun 23, 2024

🐰Bencher

ReportSun, June 23, 2024 at 12:23:07 UTC
Projecttailcall
Branch2261/merge
Testbedbenchmarking-runner
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
from_json_bench✅ (view plot)7,214,400.00 (+1.25%)7,258,797.36 (99.39%)
group_by✅ (view plot)545.22 (-4.26%)637.24 (85.56%)
input/args.missing✅ (view plot)24.97 (+3.55%)27.39 (91.16%)
input/args.nested.existing✅ (view plot)52.88 (+10.72%)62.14 (85.09%)
input/args.nested.missing✅ (view plot)36.68 (-3.69%)40.92 (89.63%)
input/args.root✅ (view plot)50.93 (+15.23%)59.36 (85.80%)
input/headers.existing✅ (view plot)32.24 (+1.93%)33.79 (95.42%)
input/headers.missing✅ (view plot)31.60 (+2.87%)33.62 (94.00%)
input/value.missing✅ (view plot)23.17 (-1.17%)25.00 (92.69%)
input/value.nested.existing✅ (view plot)41.06 (-1.16%)45.07 (91.10%)
input/value.nested.missing✅ (view plot)34.65 (-5.27%)38.69 (89.56%)
input/value.root✅ (view plot)37.35 (-1.96%)41.59 (89.80%)
input/vars.existing✅ (view plot)8.17 (+4.66%)8.94 (91.43%)
input/vars.missing✅ (view plot)11.35 (+15.62%)13.29 (85.37%)
synth_nested✅ (view plot)21,156.00 (+0.35%)21,857.10 (96.79%)
test_batched_body✅ (view plot)2,550.30 (-99.28%)1,791,281.98 (0.14%)
test_batched_body #2✅ (view plot)1,703,100.00 (+0.07%)1,825,646.11 (93.29%)
test_data_loader✅ (view plot)470,730.00 (+0.33%)486,068.19 (96.84%)
test_handle_request✅ (view plot)154,420.00 (-1.23%)172,331.71 (89.61%)
test_http_execute_method✅ (view plot)17,692.00 (-2.77%)19,290.69 (91.71%)
with_mustache_expressions✅ (view plot)1,149.50 (-1.45%)1,230.92 (93.39%)
with_mustache_literal✅ (view plot)743.56 (+3.05%)775.74 (95.85%)

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

@tusharmath tusharmath removed the ci: benchmark Runs benchmarks label Jun 23, 2024
impl ConfigModule {
pub fn config_mut(&mut self) -> &mut Config {
&mut self.config
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop this method, instead add a modify method

fn modify(f: Fn(Config) -> Config) -> Self {
  ...
}

The modify method will re-create all the internal fields such as input_types and output_types etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other option is create set_config which will recompute everything, that might be better approach.

pub fn config_mut(&mut self) -> &mut Config {
&mut self.config
}
pub fn set_extensions(&mut self, extensions: Extensions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn set_extensions(&mut self, extensions: Extensions) {
pub fn with_extensions(&mut self, extensions: Extensions) -> Self {

@tusharmath
Copy link
Contributor

Copy of the PR here — https://github.com/tailcallhq/tailcall/pull/2260/commits

@tusharmath tusharmath closed this Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim type: chore Routine tasks like conversions, reorganization, and maintenance work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: Drop pub from all the fields of ConfigModule
2 participants