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

Ensure naming of files during app and dockerfile generation is consistent #1918

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kathackeray
Copy link
Contributor

Summary

  • Use Mojo::Util::class_to_file() for file names generated from class names
  • Update documentation

Currently, if generating an app named MyApp::HTMLBuilder and its dockerfile, the following inconsistent naming is formed:

$ mojo generate app MyApp::HTMLBuilder
$ cd my_app_htmlbuilder && ./script/my_app_htmlbuilder generate dockerfile
Name Translation Uses
App directory my_app_htmlbuilder class_to_file()
App script name my_app_htmlbuilder class_to_file()
App config name my_app-h_t_m_l_builder.yml decamelize()
Dockerfile content refs my_app-h_t_m_l_builder decamelize()

This PR ensures only that class_to_file() is used consistently.

Important - This change will break existing apps!

For an app generated before this change, the former, inconsistent naming of the files will be in place.

$ ./script/my_app_original_app
Configuration file "/home/kat/tmp/~/mojo-app-gen-test/my_app_original_app/my_app_original_app.yml" missing, maybe you need to create it?

Steps for fixing:

  • Rename the config file from my_app-original_app.yml to my_app_original_app.yml
  • Regenerate or edit existing Dockerfiles to the changed naming for config

Motivation

The goal of this PR is to ensure that naming files based on class names is predictable, by utilising only the class_to_file() function for this purpose - not mixing between that and decamelize().

Whilst there are other improvements to be made, I have left these to future PRs to avoid including too much change here. Once usage is standardised, the following further improvements might be considered.

  • Ensure generated dockerfiles work out-of-the-box
  • Consider adopting generic naming within the app dir (suggested by @jberger in Generating Dockerfile refers to wrong script name #1905)
  • Modify class_to_file() to produce unambiguous and reversible file names
  • Modify decamelize() to produce output that is reversible with camelize()
  • Validate that class names passed to class_to_file() are valid

References

#1905 - Generating Dockerfile refers to wrong script name

@@ -35,7 +35,7 @@ has log => sub {
};
has 'max_request_size';
has mode => sub { $ENV{MOJO_MODE} || $ENV{PLACK_ENV} || 'development' };
has moniker => sub { Mojo::Util::decamelize ref shift };
has moniker => sub { Mojo::Util::class_to_file ref shift };
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change unfortunately. This has implications about how configuration files are loaded by default. I'd be fine changing this at a major version or at least in line with our breaking changes rules (though I would personally rather this be at a major version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a process I should follow to put this forward for a major release?

Copy link
Member

Choose a reason for hiding this comment

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

While I support your stance @jberger that this is a breaking change and thus should be planned for a major release, it is also a question of definition: is the proposed change a change or a bugfix?

When looking over a couple of projects here I see that we usually end up overriding moniker because of how the default moniker is generated (see example table from the PR description). So from that perspective if this change was to be implemented we no longer would have to set moniker in our apps, because the problem with the auto-generated moniker would go away.

From that perspective I think it is fair to view this as a bugfix, and thus be considered for a minor-release.

@kathackeray kathackeray force-pushed the consistent-naming-during-app-gen branch from 0605239 to e57c2ad Compare February 21, 2022 18:49
@kathackeray
Copy link
Contributor Author

This proposed first step to resolution of #1905 has failed review on account of a breaking change. As any clean fix for this issue would necessitate a breaking change, I can't see a way to push this PR through. As such I'm closing.

@kraih
Copy link
Member

kraih commented Mar 9, 2022

This PR has not failed review. There was just one objection.

@kathackeray
Copy link
Contributor Author

Noted on the closure: I didn't know how to proceed and wanted to avoid clogging the PR list. I'll be more patient.

@kathackeray kathackeray reopened this Mar 9, 2022
@tianon
Copy link
Contributor

tianon commented Mar 9, 2022

I'm not a maintainer, but IMO this is the cleanest solution to the problem - having two different ways to lowercase the app class name that both end up in the filesystem almost next to each other seems like it's worth making a breaking change next release (especially since I imagine the number of users affected is not huge, but even if it is, the discrepancy is kind of an eyesore).

Copy link
Member

@christopherraa christopherraa left a comment

Choose a reason for hiding this comment

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

This looks good to me. Though I would like to see others weigh with their opinions on this is considered a bug or an ordinary change. See my reply to the comment made by @jberger .

@kraih kraih requested review from a team, marcusramberg, jberger and Grinnz June 14, 2023 09:55
@marcusramberg
Copy link
Member

I'm really unsure what to think about this. I'm leaning towards jberger's view that we should fix this in a major release.

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

6 participants