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

Console app on convert #169

Merged
merged 6 commits into from
Dec 31, 2024
Merged

Conversation

logan-keede
Copy link
Contributor

@logan-keede logan-keede commented Dec 26, 2024

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

to add console_app in template

Refs beeware/briefcase#1900 beeware/briefcase#2089

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Two notes (with possible corrections inline):

  1. Is the attribute name (as rendered in the template) correct?
  2. Should this be a string, or a boolean? The problem is that "False" will evaluate as true.

@@ -28,6 +28,7 @@
],
"app_source": "",
"app_start_source": "",
"console_app": "",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"console_app": "",
"console_app": false,

@@ -19,6 +19,9 @@ sources = [
test_sources = [
"{{ cookiecutter.test_source_dir }}",
]
{% if cookiecutter.console_app %}
console.app = "{{cookiecutter.console_app}}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.app = "{{cookiecutter.console_app}}"
console_app = {{ cookiecutter.console_app }}

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 think the if statement should stay to avoid clash between new and convert commands, new command uses pyproject_table_briefcase_app_extra_content attribute while convert command will use console_app attribute, if we do not use if statement we can have console_app statement twice in pyproject.toml, or we can refactor new command to use this attribute.

Copy link
Member

Choose a reason for hiding this comment

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

I think you've misunderstood the suggestion I'm making here. I agree that the if statement should be retained; the suggestion is that console.app (period, rather than underscore) is the wrong key to use, and the value shouldn't be quoted.

@logan-keede logan-keede force-pushed the console_app_on_convert branch from b3509b6 to 1fefb9a Compare December 29, 2024 13:00
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Thanks - this looks great!

@freakboy3742 freakboy3742 merged commit af39711 into beeware:main Dec 31, 2024
26 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.

2 participants