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

shell: add 'exit' builtin command #9705

Merged
merged 5 commits into from
Mar 30, 2024
Merged

shell: add 'exit' builtin command #9705

merged 5 commits into from
Mar 30, 2024

Conversation

nektro
Copy link
Member

@nektro nektro commented Mar 29, 2024

breaking out #9578 into multiple PRs in order to better pin down where my CI is going awry

edit: progress on #9716

Copy link
Contributor

github-actions bot commented Mar 29, 2024

Copy link
Contributor

github-actions bot commented Mar 29, 2024

Copy link
Contributor

github-actions bot commented Mar 29, 2024

❌🪟 @nektro, there are 12 test regressions on Windows x86_64

  • test\cli\install\bunx.test.ts
  • test\cli\install\registry\bun-install-registry.test.ts
  • test\js\bun\console\console-iterator.test.ts
  • test\js\bun\glob\scan.test.ts
  • test\js\bun\spawn\spawn-empty-arrayBufferOrBlob.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\stream\node-stream.test.js
  • test\js\third_party\esbuild\esbuild-child_process.test.ts
  • test\js\web\fetch\body-stream.test.ts
  • test\regression\issue\09041.test.ts
  • test\js\web\workers\worker.test.ts
  • test\js\third_party\postgres\postgres.test.ts

Full Test Output

Copy link
Contributor

github-actions bot commented Mar 29, 2024

@gvilums gvilums self-requested a review March 29, 2024 19:03
Copy link
Contributor

@gvilums gvilums left a comment

Choose a reason for hiding this comment

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

I assume the failing tests are just general flakiness

Comment on lines +4223 to +4226
inline else => |tag| {
cmd.exec.bltn.impl = @unionInit(RealImpl, @tagName(tag), .{
.bltn = &cmd.exec.bltn,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly not sure how I feel about this, it's definitely more concise than what we had previously but also makes it harder to understand what exactly is going on IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

could you elaborate on this a bit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

personally, i like it

it also encourages all the commands have the same interface

src/shell/interpreter.zig Outdated Show resolved Hide resolved
}

pub fn deinit(this: *Exit) void {
_ = this;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the memory owned by the parent struct?

Copy link
Member Author

Choose a reason for hiding this comment

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

appears so checking all the other builtins

@Jarred-Sumner Jarred-Sumner merged commit b8389f3 into main Mar 30, 2024
26 of 31 checks passed
@Jarred-Sumner Jarred-Sumner deleted the nektro-patch-25869 branch March 30, 2024 01:20
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.

3 participants