Skip to content

feat(core): extract server safe core#230

Merged
fahimfaisaal merged 8 commits into
goptics:mainfrom
hfl0506:feat/server-safe-core
Jul 18, 2026
Merged

feat(core): extract server safe core#230
fahimfaisaal merged 8 commits into
goptics:mainfrom
hfl0506:feat/server-safe-core

Conversation

@hfl0506

@hfl0506 hfl0506 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Related Issue

Resolve #212

Changes

  • Added server-safe core operations for inline conversion, Dataset assembly,
    merging, and UI generation.

  • Converted CSV/JSON parsing to reader-based, error-returning paths.

  • Replaced server-reachable parser select/value exits with returned errors.

  • Added quiet auto-detection for request paths to avoid terminal output.

  • Added error-returning HTML template generation APIs.

  • Updated CLI merge, UI, and Dataset assembly to use shared core operations.

  • Added core success, validation, chart, merge, UI, and 3D coverage tests.

  • Raised pkg/core coverage to 93.0%.

Test Steps

task format:check

# for core coverage
go test -cover ./pkg/core

go test ./...

@codecov-commenter

codecov-commenter commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@hfl0506
hfl0506 marked this pull request as ready for review July 17, 2026 05:34
@hfl0506
hfl0506 requested a review from fahimfaisaal July 17, 2026 07:00
@hfl0506 hfl0506 added enhancement New feature or request cli Command line interface related tasks labels Jul 17, 2026
fahimfaisaal
fahimfaisaal requested changes Jul 17, 2026
View reviewed changes
Comment thread pkg/parser/registry.go Outdated
fahimfaisaal
fahimfaisaal requested changes Jul 17, 2026
View reviewed changes
Comment thread cmd/cli/pipeline.go
@@ -477,15 +478,24 @@ func autoEnableValueMode3D(configs []internal_charts.ChartConfig, axes []shared.
// assembleDataset builds the output Dataset from parsed results plus the
// command's metadata and the resolved per-chart configs.
func assembleDataset(results []shared.DataPoint, m RunMeta, configs []internal_charts.ChartConfig, cfg parser.Config) *shared.Dataset {

@fahimfaisaal fahimfaisaal Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

assembleDataset already goes through core. Assemble, but the old helpers (buildDataset and friends) are still sitting next to it as a second copy. A few tests still call that old path, so they miss the code we actually ship. Can we delete the CLI copies and point those tests at assembleDataset / core.Assemble?

Comment thread pkg/template/generate-ui.go Outdated
@fahimfaisaal fahimfaisaal added this to the Release v0.16.0 milestone Jul 17, 2026
@hfl0506
hfl0506 force-pushed the feat/server-safe-core branch from 4295604 to f85685a Compare July 18, 2026 02:39
@hfl0506

hfl0506 commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @fahimfaisaal I do a refactoring based on your comments, please take a look when you have a chance. Thanks

@fahimfaisaal

fahimfaisaal commented Jul 18, 2026

Copy link
Copy Markdown
Member

Hi @hfl0506 Thanks for the chnages, Well done for moving parser errors into returned errors. One server-safety issue remains: ParseGoBenchmark still writes OS, architecture, package, and CPU data to shared globals. Concurrent requests could race and mix metadata. Could we return this metadata from the parser and pass it into core.Assemble instead, with a concurrent race test to cover it?

@fahimfaisaal fahimfaisaal mentioned this pull request Jul 18, 2026
Closed
@fahimfaisaal

fahimfaisaal commented Jul 18, 2026

Copy link
Copy Markdown
Member

🥷 Let’s handle the request-scoped Go benchmark metadata in #236. This PR already contains a large set of changes, so keeping this follow-up in a separate merge should make both reviews easier.

@fahimfaisaal
fahimfaisaal merged commit 9f068e9 into goptics:main Jul 18, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fahimfaisaal fahimfaisaal fahimfaisaal approved these changes

Assignees

No one assigned

Labels

cli Command line interface related tasks enhancement New feature or request

Projects

None yet

Milestone

Release v0.16.0

Development

Successfully merging this pull request may close these issues.

extract a server-safe Vizb core

3 participants

@hfl0506 @codecov-commenter @fahimfaisaal