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

Move NodeId Generation to a separate crate #484

Merged
merged 4 commits into from
Aug 9, 2024
Merged

Move NodeId Generation to a separate crate #484

merged 4 commits into from
Aug 9, 2024

Conversation

am357
Copy link
Contributor

@am357 am357 commented Aug 8, 2024

Description of changes:

Moves the NodeId and its Id generation to a new crate, partiql-common so that it can get consumed by multiple crates.

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

@am357 am357 force-pushed the partiql-core branch 2 times, most recently from bc8527a to b947876 Compare August 8, 2024 19:16
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.00%. Comparing base (6dc088c) to head (b7a8fa0).

Files Patch % Lines
partiql-common/src/node.rs 91.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #484   +/-   ##
=======================================
  Coverage   81.00%   81.00%           
=======================================
  Files          68       69    +1     
  Lines       18704    18704           
  Branches    18704    18704           
=======================================
  Hits        15152    15152           
  Misses       3110     3110           
  Partials      442      442           

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

Copy link

github-actions bot commented Aug 8, 2024

Conformance comparison report

Base (6dc088c) 6bd053c +/-
% Passing 90.35% 90.35% 0.00%
✅ Passing 5731 5731 0
❌ Failing 612 612 0
🔶 Ignored 0 0 0
Total Tests 6343 6343 0

Number passing in both: 5731

Number failing in both: 612

Number passing in Base (6dc088c) but now fail: 0

Number failing in Base (6dc088c) but now pass: 0

@am357 am357 marked this pull request as ready for review August 8, 2024 20:22
@am357 am357 requested a review from jpschorr August 8, 2024 20:22
Copy link
Contributor

@jpschorr jpschorr left a comment

Choose a reason for hiding this comment

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

I wonder if there is a better name than partiql-core?

Perhaps we could move this type and partiql-source-map somewhere...

@am357
Copy link
Contributor Author

am357 commented Aug 8, 2024

I wonder if there is a better name than partiql-core?

Not sure TBH—happy to change if you have a suggestion.

Perhaps we could move this type and partiql-source-map somewhere...

I could see partiql-source-map going under: <partiql-core>/src/syntax-source/.... Thoughts?

@jpschorr
Copy link
Contributor

jpschorr commented Aug 8, 2024

I wonder if there is a better name than partiql-core?

Not sure TBH—happy to change if you have a suggestion.

We talked out-of-band and thing partiql-common may be less ambiguous.

Perhaps we could move this type and partiql-source-map somewhere...

I could see partiql-source-map going under: <partiql-core>/src/syntax-source/.... Thoughts?

Sure

@am357 am357 changed the title Move NodeId Generation to partiql-core::utils Move NodeId Generation to partiql-common::node Aug 9, 2024
@am357 am357 changed the title Move NodeId Generation to partiql-common::node Move NodeId Generation to a separate crate Aug 9, 2024
@am357 am357 merged commit e5b8ce7 into main Aug 9, 2024
19 checks passed
@am357 am357 deleted the partiql-core branch August 9, 2024 01:39
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