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

Use partiql_ast::ast::AstTypeMap for LocationMap #394

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

am357
Copy link
Contributor

@am357 am357 commented Jun 14, 2023

Description of changes:
Addressing the comment in PR #389, this PR re-uses the AstTypeMap for LocationMap which removes a dependency to HashMap as AstTypeMap uses IndexMap.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

am357 added 2 commits June 14, 2023 12:10
Addressing the comment in PR #389, this PR re-uses the `AstTypeMap`
for `LocationMap` which removes a dependency to `HashMap` as `AstTypeMap`
uses `IndexMap`.
@am357 am357 marked this pull request as ready for review June 14, 2023 19:16
@am357 am357 requested review from jpschorr and alancai98 June 14, 2023 19:16
@@ -20,7 +20,7 @@ path = "src/lib.rs"
bench = false

[dependencies]
indexmap = "1.9"
indexmap = { version = "1.9", default-features = false, features = ["serde"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to remove the unconditional features = ["serde"], since you turn it on below conditionally via serde = [... "indexmap/serde"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, let fix that.

@github-actions
Copy link

github-actions bot commented Jun 14, 2023

Conformance comparison report

Base (babd7ac) f7be104 +/-
% Passing 83.39% 83.39% 0.00%
✅ Passing 5240 5240 0
❌ Failing 1044 1044 0
🔶 Ignored 0 0 0
Total Tests 6284 6284 0

Number passing in both: 5240

Number failing in both: 1044

Number passing in Base (babd7ac) but now fail: 0

Number failing in Base (babd7ac) but now pass: 0

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (babd7ac) 81.44% compared to head (e77d04b) 81.44%.

❗ Current head e77d04b differs from pull request most recent head 5890532. Consider uploading reports for the commit 5890532 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #394   +/-   ##
=======================================
  Coverage   81.44%   81.44%           
=======================================
  Files          56       56           
  Lines       14667    14667           
  Branches    14667    14667           
=======================================
  Hits        11945    11945           
  Misses       2214     2214           
  Partials      508      508           
Impacted Files Coverage Δ
partiql-parser/src/lib.rs 86.95% <ø> (ø)
partiql-parser/src/parse/mod.rs 94.76% <ø> (ø)
partiql-parser/src/parse/parser_state.rs 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@am357 am357 merged commit 004d2c0 into main Jun 14, 2023
@am357 am357 deleted the feat-locationmap-typemap branch June 14, 2023 20:32
@am357 am357 mentioned this pull request Jun 14, 2023
6 tasks
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.

2 participants