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

CRM-21765: Commit files to git which are static but are generated from CodeGen like DAO files #11667

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Feb 14, 2018

Overview

Add SchemaStructure.php and langs.php to git, reducing the need to run GenCode.

Before

  • The files SchemaStructure.php and langs.php do not exist in git.
  • To get these files, you must either download the tarball or run GenCode.

After

  • The files SchemaStructure.php and langs.php are stored in git.
  • If you run GenCode, then these files will be updated. However, you generally don't need to update them.

@monishdeb monishdeb force-pushed the CRM-21765 branch 2 times, most recently from 5016ef2 to 6cb38ae Compare February 14, 2018 18:38
@totten
Copy link
Member

totten commented Feb 14, 2018

Ah, the test raises an interesting point that the generated code doesn't pass style guidelines. We haven't noticed because the file was never checked by CI before.

In theory, GenCode has a reformatting mechanism (part of CRM_Core_CodeGen_Util_Template) to address style issues, but it's not perfect. You could fiddle with the template or the formatter, but..

It's probably easier to grant an exception for auto-generated code. We did that a long time ago for DAO's, like this: https://github.com/civicrm/civicrm-buildkit/blob/f55cdde/bin/civilint#L54-L55

@monishdeb
Copy link
Member Author

monishdeb commented Feb 14, 2018

Thanks @totten I have now created a PR civicrm/civicrm-buildkit#392 to skip those two files from civilint check

@totten
Copy link
Member

totten commented Feb 15, 2018

OK, merged the update for civilint.

Before we had the civilint exception, did you by chance try manual edits on the CRM/Core/I18n/SchemaStructure.php and install/langs.php? The reason I ask... after running GenCode locally, I wind up with slightly different files. (And the differences look like style fixups.) But I think it'll get confusing if the committed files differ from the generated files.

Maybe go back to the unedited files? Or maybe update xml/templates/schema_structure.tpl to make the edits more durable?

@monishdeb
Copy link
Member Author

@totten yes you are right I did manual edit before I got your comment. So now I have regenerated those two files and updated the patch. Hopefully civilint doesn't pick up the style error this time :)

@totten
Copy link
Member

totten commented Feb 15, 2018

Thanks, @monishdeb ! The results are now consistent across multiple runs of GenCode.

@totten totten merged commit dac3ef1 into civicrm:master Feb 15, 2018
@monishdeb monishdeb deleted the CRM-21765 branch February 26, 2018 04:05
@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants