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

Advanced flag #149

Merged
merged 31 commits into from
Dec 26, 2023
Merged

Advanced flag #149

merged 31 commits into from
Dec 26, 2023

Conversation

briancbarrow
Copy link
Collaborator

@briancbarrow briancbarrow commented Nov 30, 2023

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

Problem/Feature

This introduces the --advanced flag discussed in #147.

Description of Changes:

  • This adds the advanced flag to the available options when initializing a new project. The presence of the flag pushes the user into the "advanced" prompts. Currently, the only advanced option is adding HTMX/Templ support.

As of right now when creating the draft PR, the HTMX/Templ support is incomplete. Below is a list of places it needs to be added. I wanted to get the draft PR open in case others wanted to start adding to it. I'll keep adding to it the rest of the week/weekend and hopefully we can get something ready to merge by next week.

routes

  • http_router
  • chi
  • echo
  • fiber
  • gin
  • gorilla
  • standard_library

dbRoutes

  • http_router
  • chi
  • echo
  • fiber
  • gin
  • gorilla
  • standard_library

Checklist

  • I have self-reviewed the changes being requested
  • I have updated the documentation (if applicable)

@briancbarrow briancbarrow changed the base branch from staging to staging-2 November 30, 2023 14:24
@Ujstor Ujstor mentioned this pull request Dec 1, 2023
1 task
@briancbarrow briancbarrow marked this pull request as ready for review December 4, 2023 12:41
@Ujstor Ujstor requested a review from Melkeydev December 5, 2023 04:24
@Ujstor
Copy link
Collaborator

Ujstor commented Dec 5, 2023

This is PROD-ready, there is no need to merge into the staging branch.

cmd/create.go Outdated Show resolved Hide resolved
cmd/create.go Outdated Show resolved Hide resolved
cmd/steps/steps.go Outdated Show resolved Hide resolved
cmd/template/advanced/files/htmx/base.templ.tmpl Outdated Show resolved Hide resolved
cmd/program/program.go Outdated Show resolved Hide resolved
Comment on lines 680 to 683
if err != nil {
log.Fatal(err)
}
Copy link
Owner

Choose a reason for hiding this comment

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

if placehodler does not get re-written, is it going to log fatal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When running through it with the flag being false, it doesn't not log fatal. The Parse is just taking in a string so I think it'll be fine no matter what the string is, right?

cmd/program/program.go Outdated Show resolved Hide resolved
cmd/ui/multiSelect/multiSelect.go Outdated Show resolved Hide resolved
cmd/ui/multiSelect/multiSelect.go Outdated Show resolved Hide resolved
cmd/ui/booleanInput/booleanInput.go Outdated Show resolved Hide resolved
Copy link
Owner

@Melkeydev Melkeydev left a comment

Choose a reason for hiding this comment

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

Current process cannot be killed

cmd/create.go Show resolved Hide resolved
}
err = spinner.ReleaseTerminal()
if err != nil {
log.Printf("Could not release terminal: %v", err)
Copy link
Owner

Choose a reason for hiding this comment

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

nit:
if we get here, cant we make sure to release the terminal for them? Maybe explicitly setting a kill for the terminal? Otherwise it will be hanging and the user needs to close the terminal.
Not blocking, just some food for thought

Copy link
Collaborator Author

@briancbarrow briancbarrow Dec 23, 2023

Choose a reason for hiding this comment

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

What would that look like? Would we want to do an os.Exit or something like that?

Won't the CheckErr handle exiting with a non-zero exit code?

Sorry if these are simple questions.

Copy link
Owner

@Melkeydev Melkeydev left a comment

Choose a reason for hiding this comment

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

LGTM.
Major release 0.5.0!!!!!
I will let you merge when you're ready
@Ujstor and @limesten - if you guys want to throw approvals on this too

Copy link
Owner

@Melkeydev Melkeydev left a comment

Choose a reason for hiding this comment

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

Sorry, revoking my approval.
Can we change this from staging-2 to main branch?

@briancbarrow briancbarrow changed the base branch from staging-2 to main December 22, 2023 23:00
Copy link
Collaborator

@Ujstor Ujstor left a comment

Choose a reason for hiding this comment

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

Besides the conflict, LGTM.

cmd/program/program.go Show resolved Hide resolved
# Conflicts:
#	cmd/program/program.go
Copy link
Owner

@Melkeydev Melkeydev left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@Ujstor Ujstor left a comment

Choose a reason for hiding this comment

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

LGTM

@Melkeydev Melkeydev merged commit b534117 into Melkeydev:main Dec 26, 2023
37 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.

4 participants