mojo

mojo copied to clipboard

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 #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

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.

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

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

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).

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.