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

feat: integrating numscript rewrite #503

Merged
merged 7 commits into from
Oct 23, 2024
Merged

Conversation

ascandone
Copy link
Contributor

@ascandone ascandone commented Oct 2, 2024

Changelog:

The history is a bit messed up after the last rebase I did but should mostly make sense so you can read commit by commit

@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch 2 times, most recently from aeb468e to 75d32fc Compare October 3, 2024 12:47
@ascandone ascandone changed the title integrating numscript rewrite feat: integrating numscript rewrite Oct 3, 2024
@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch 3 times, most recently from 76ef6f9 to 7f97a02 Compare October 9, 2024 13:25
@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch 8 times, most recently from 19412d8 to e2ed5b9 Compare October 16, 2024 09:38
@gfyrag gfyrag force-pushed the feat/integrate-numscript-rewrite branch from e2ed5b9 to 9d4e597 Compare October 16, 2024 09:45
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 78.35052% with 21 lines in your changes missing coverage. Please review.

Project coverage is 82.75%. Comparing base (e5594ac) to head (bc861fc).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/controller/ledger/store.go 30.43% 15 Missing and 1 partial ⚠️
internal/controller/ledger/numscript_runtime.go 89.28% 2 Missing and 1 partial ⚠️
internal/api/v2/controllers_bulk.go 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #503      +/-   ##
==========================================
- Coverage   82.79%   82.75%   -0.05%     
==========================================
  Files         117      117              
  Lines        6773     6860      +87     
==========================================
+ Hits         5608     5677      +69     
- Misses        881      898      +17     
- Partials      284      285       +1     

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

@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch 12 times, most recently from 5133f60 to 594e139 Compare October 22, 2024 09:18
@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch 7 times, most recently from a96a496 to 72e6de6 Compare October 22, 2024 18:23
@ascandone ascandone requested a review from gfyrag October 22, 2024 18:27
@ascandone ascandone marked this pull request as ready for review October 22, 2024 18:27
@ascandone ascandone requested a review from a team as a code owner October 22, 2024 18:27
Base automatically changed from feat/ledger-stateless to main October 23, 2024 08:13
})
When("creating a bulk on a ledger", func() {
var (
now = time.Now().Round(time.Microsecond).UTC()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal but there is wrapper around time package in go-libs, which already round dates at microseconds (postgres precision)

Copy link
Contributor

Choose a reason for hiding this comment

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

and set UTC too

Copy link
Contributor

@gfyrag gfyrag left a comment

Choose a reason for hiding this comment

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

Approved but i would be grateful if you can change the decorating stuff around the parser.
Keep the option simple, and make the plumber in the module.go file.

@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch 2 times, most recently from 56191fd to b4fd1b9 Compare October 23, 2024 14:52
@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch from b4fd1b9 to 056b89c Compare October 23, 2024 15:21
@ascandone ascandone force-pushed the feat/integrate-numscript-rewrite branch from a148911 to bc861fc Compare October 23, 2024 15:37
}{
{"default", false},
{"numscript rewrite", true},
} {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the only diff is that I wrapped everything in this for loop

Debug: debug,
NatsURL: natsServer.GetValue().ClientURL(),
BulkMaxSize: bulkMaxSize,
ExperimentalNumscriptRewrite: data.numscriptRewrite,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that here it's using the feature flag value

}{
{"default", false},
{"numscript rewrite", true},
} {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as before, here the only diff is wrapping all in a loop

@ascandone ascandone merged commit 223b4c4 into main Oct 23, 2024
7 of 9 checks passed
@ascandone ascandone deleted the feat/integrate-numscript-rewrite branch October 23, 2024 16:54
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