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

Refactor jokes jbang bot to be picocli based #571

Merged
merged 1 commit into from
May 16, 2024

Conversation

iocanel
Copy link
Collaborator

@iocanel iocanel commented May 11, 2024

The pull request changes the jbang jokes bot, to be a cli application instead of a web application.

The reasoning is that we already have a lot of samples going the web path and it seems to me like a missed opportunity to show case to also demonstrate how it can be done in the CLI.

Now, I understand that the reasoning might be to implement something with the fewer lines of code possible, but I argue that the code is hard to understand.

@iocanel iocanel requested a review from a team as a code owner May 11, 2024 13:04
@iocanel
Copy link
Collaborator Author

iocanel commented May 11, 2024

cc @maxandersen

@geoand geoand requested a review from jmartisk May 11, 2024 13:06
@jmartisk
Copy link
Collaborator

I'm not completely sure I like this TBH, first it has gotten bigger (from 13 to 20 lines), second, unless it showcases some actual CLI stuff (like parameters) then CLI is unnecessary (and the point is to show a LLM app, not a CLI app). But I'll leave it up to you guys

@iocanel
Copy link
Collaborator Author

iocanel commented May 13, 2024

Yeah, the number of lines has increased, but it's mostly metadata, annotations and imports. One the other hand it's less dense and thus way easier to understand. It's also much simpler to run, as it does in a single step.

It's also a more realistic use case of jbang scripting.

@cescoffier
Copy link
Collaborator

@iocanel Can you fix the conflict?

@geoand geoand merged commit a7fdc7d into quarkiverse:main May 16, 2024
12 checks passed
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.

None yet

4 participants