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

Bring back AsyncCommands #3109

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Bring back AsyncCommands #3109

merged 4 commits into from
Dec 1, 2023

Conversation

marius-se
Copy link
Contributor

@marius-se marius-se commented Dec 1, 2023

These changes are now available in 4.88.0

Brings async commands back by adding a new property asyncCommands to Application.

@marius-se
Copy link
Contributor Author

I just saw my formatter went for a little rampage in Application.swift, I hope that's not a problem :D

@marius-se
Copy link
Contributor Author

Depends on: vapor/console-kit#195

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

One nit and we need to update both package manifests to point to 4.13.0

Sources/Vapor/Application.swift Outdated Show resolved Hide resolved
@@ -77,6 +83,7 @@ extension Application {
var commands = Commands()
commands.use(BootCommand(), as: "boot")
self.commands = .init(commands)
self.asyncCommands = .init(AsyncCommands())
Copy link
Member

Choose a reason for hiding this comment

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

Tempted to say we should switch all the internals over but I think that can wait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was also thinking about that, but I'd say that belongs to a separate PR. 👍🏼

Co-authored-by: Tim Condon <0xTim@users.noreply.github.com>
@0xTim 0xTim added the semver-minor Contains new API label Dec 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

Merging #3109 (5f8c5b3) into main (da9c280) will increase coverage by 0.04%.
The diff coverage is 95.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3109      +/-   ##
==========================================
+ Coverage   76.37%   76.41%   +0.04%     
==========================================
  Files         211      211              
  Lines        8017     8031      +14     
==========================================
+ Hits         6123     6137      +14     
  Misses       1894     1894              
Files Coverage Δ
Sources/Vapor/Application.swift 92.53% <100.00%> (+0.66%) ⬆️
Sources/Vapor/Core/Core.swift 70.83% <75.00%> (+1.94%) ⬆️


app.asyncCommands.use(FooCommand(), as: "foo")

app.environment.arguments = ["vapor", "foo", "bar"]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
app.environment.arguments = ["vapor", "foo", "bar"]
app.environment.arguments = ["vapor", "foo", "--name", "bar"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's an @Argument not @Option


app.environment.arguments = ["vapor", "foo", "bar"]

XCTAssertNoThrow(try app.start())
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way we can verify it runs, like incrementing a global counter? A manual run will be good enough if you haven't done that. (Do we have any tests for normal commands)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I just pushed a commit. Unfortunately we don't have any tests for commands (or I couldn't find one)

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

🥳

@0xTim 0xTim enabled auto-merge (squash) December 1, 2023 22:05
@0xTim 0xTim merged commit c710b8f into main Dec 1, 2023
17 checks passed
@0xTim 0xTim deleted the feature/async_commands branch December 1, 2023 22:08
@patrick-zippenfenig
Copy link
Contributor

Much appreciated! I can finally remove my workaround to use async in commands 🎉🎉🎉

References to other PRs #2870 #2853 vapor/console-kit#175 vapor/console-kit#181

keniwhat pushed a commit to keniwhat/vapor that referenced this pull request Jan 19, 2024
* main: (44 commits)
  Update routing-kit version (vapor#3131)
  Use `singleton` `EventLoopGroup` (vapor#3128)
  Additional tests fixes
  Fix for Apple changing things without warning.
  Add missing availability annotations in URI tests
  Merge pull request from GHSA-r6r4-5pr8-gjcp
  Fix setting public folder for `FileMiddleware` when using bundles (vapor#3113)
  Consistently use the value from `X-Request-Id` as the request's ID when present (vapor#3117)
  Fix encoding and decoding of HTTPHeaders (vapor#3116)
  Add fully async entrypoints (vapor#3114)
  Bring back AsyncCommands (vapor#3109)
  General warnings and tests cleanup (vapor#3107)
  Add public initializer for `XCTHTTPRequest` (vapor#3106)
  [skip ci] Update dependabot.yml
  [skip ci] Fixup sponsors workflow
  Make Async Request Body actually work (vapor#3096)
  Create a thread pool of System.coreCount rather than 64 when initializing an Application (vapor#3092)
  Modernize sponsors automation workflow (vapor#3098)
  Bump the dependencies group with 2 updates (vapor#3099)
  Update README with new Sponsor (vapor#3101)
  ...

# Conflicts:
#	Package.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Contains new API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants