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.

Sign up for GitHub

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

Jump to bottom

Move to the forked Sobek code #3792

Merged
merged 2 commits into from
Jun 26, 2024
Merged

Move to the forked Sobek code #3792

merged 2 commits into from
Jun 26, 2024

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Jun 11, 2024 •
edited
Loading

What?

Move to Grafana fork of goja called Sobek - now with the actual code not just aliases

Why?

See #3772

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@mstoykov mstoykov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Jun 11, 2024
@mstoykov mstoykov added this to the v0.53.0 milestone Jun 11, 2024
@mstoykov mstoykov requested a review from a team as a code owner June 11, 2024 09:11
@mstoykov mstoykov requested review from oleiade and codebien and removed request for a team June 11, 2024 09:11
codebien
codebien previously approved these changes Jun 11, 2024
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.87%. Comparing base (20f8feb) to head (3320b05).

❗ Current head 3320b05 differs from pull request most recent head 93c0c36

Please upload reports for the commit 93c0c36 to get more accurate results.

Files Patch % Lines
js/modules/k6/html/html.go 90.90% 0 Missing and 1 partial ⚠️
js/runner.go 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3792   +/-   ##
=======================================
  Coverage   71.87%   71.87%           
=======================================
  Files         291      291           
  Lines       21274    21274           
=======================================
+ Hits        15290    15291    +1     
+ Misses       4920     4919    -1     
  Partials     1064     1064
Flag Coverage Δ
ubuntu 71.79% <89.47%> (-0.02%) ⬇️
windows 71.73% <89.47%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

oleiade
oleiade previously approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🎉 🚀

codebien
codebien previously approved these changes Jun 11, 2024
View reviewed changes
@mstoykov mstoykov dismissed stale reviews from codebien and oleiade via e76ab13 June 21, 2024 14:26
codebien
codebien reviewed Jun 21, 2024
View reviewed changes
cmd/runtime_options.go Outdated
@@ -24,7 +24,7 @@ func runtimeOptionFlagSet(includeSysEnv bool) *pflag.FlagSet {
flags.Bool("include-system-env-vars", includeSysEnv, "pass the real system environment variables to the runtime")
flags.String("compatibility-mode", "extended",
`JavaScript compiler compatibility mode, "extended" or "base" or "experimental_enhanced"
base: pure goja - Golang JS VM supporting ES5.1+
base: pure sobek - Golang JS VM supporting ES5.1+
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
base: pure sobek - Golang JS VM supporting ES5.1+
base: pure Sobek - Golang JS VM supporting ES5.1+

Can we have it with a capitalized letter? I see this across multiple lines across this pull request. Consider this suggestion for all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not strictly against this, but we do not capitalize it anywhere else as well as far as I can see - and we do not really do it for our other projects.

Copy link
Contributor

@codebien codebien Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You did it for the first file Dependencies 😅

Btw, I don't think we have other projects with a name independent from k6. All other projects have names prefixed with k6 or some combination of it like xk6 where it doesn't make sense to capitalize. But in this case, it is a proper name, so I don't see why we shouldn't as it is the correct grammar.

Of course, I expect this only for text and not for code (e.g repo, package names, etc..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made that lower case as well 😅

Copy link
Member

Choose a reason for hiding this comment

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

Wearing my linguist's cap here 🦖

I have to agree with @codebien here. I would also prefer it if we capitalized Sobek when referring to it in a non-code context (we're displaying information to the user). The reason is that it's used as a "proper noun" here, which in English are always capitalized.

For illustration, the reference to "Golang", three characters away is capitalized for instance.

@mstoykov mstoykov changed the title Move to the forked sobek code Move to the forked Sobek code Jun 26, 2024
@mstoykov
Move to the forked Sobek code
ccdf673 
There are still some leftover comments and names referencing goja that
need to be updated, but this compiles.

This closes #3772 and closes #3773
@mstoykov
Replace Sobek with goja in comments and identifiers
93c0c36 
Some places weren't applicable as they link to issues or are
acknowledging contributions.
@mstoykov mstoykov merged commit d9b5fd0 into master Jun 26, 2024
23 checks passed
@mstoykov mstoykov deleted the moveToForkedSobek branch June 26, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codebien codebien codebien approved these changes

@oleiade oleiade oleiade approved these changes

Assignees
No one assigned
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Milestone
v0.53.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mstoykov @codecov-commenter @oleiade @codebien