fix: add license and generated code headers to generated resource files

[Hectorhammett]

Change the ResourcesGenerator class to make use of the AST::file() method to add the required comments. I also added some comments for discussion on this PR to add them in a different PR if we come with a more elegant solution.

Fixes #643

[Hectorhammett]

I am adding the ; manually here but I am not particularly fond of this.

My first attempt was to add this to the return method on AST but that function is used in different places. I am using the PhpFile class which has the ability to use a codeblock but any PhpBlock class implements the ShouldNotApplySemicolonInterface which the AST::toPhp method checks to decide wether or not add a semicolon.

We could create a EndPhpBlock or FinalPhpBlock class that does not implements the interface perhaps?

AST::file(null)
    ->withBlock(new EndPhpBlock($string))

Add a method that internally forces a semicolon while calling the toPhp method?

AST::file(null)
    ->withBlock(new PhpBlock($string))
    ->withSemiColon()

perhaps a method on the PhpClass to "finalize" the class? But perhaps the word for "Final" might be confused by a Final class. Ready perhaps? that adds a ; to the end?

AST::file(null)
    ->withBlock(new PhpBlock())
    ->ready()

[noahdietz]

Interesting...I don't want to mess with PhpBlock too much, especially if it is already opinionated about semicolons. Given how choosy the AST is about semicolons altogether, I think this might be a reasonable solution to keep it how it is...or do what you suggested with a new EndPhpBlock...maybe LastPhpBlock? It just wraps a Block and tacks on the ; in toCode?

[Hectorhammett]

I poked around a bit more today to see if adding the extra PHP class would help but after debugging and poking further turns out that in this case, it is actually not using the phpBlock class, but just a regular AST class and the toPhp method just calls toCode on an AST instance:

elseif ($x instanceof AST) {
    return $x->toCode();
}

So the semicolon interface does nothing in this case lol. I think it would be best to have a way to add a semicolon codewise but for now let's keep it like it is.

[noahdietz]

So the semicolon interface does nothing in this case lol. I think it would be best to have a way to add a semicolon codewise but for now let's keep it like it is.

Nice code spelunking! I agree, simple enough for now. Maybe file a priority: p3 type: cleanup bug to look at this?

[noahdietz]

Note: I changed the title to fix so that with automerge (for merge-on-green bot), the commits are squashed and the PR title is used as the main commit message. We want this to be fix b.c it changes the generated code, but not feat because it doesn't impact the end-user surface or functionality. This is my interpretation of the Conventional Commit spec that acts as the contract between our commit types and release-please semver versioning

[noahdietz]

You will also need to update the Bazel integration test Golden files. Those can be updated locally via the instructions in DEVELOPMENT.md

[noahdietz]

When updating the integration test golden files you will need to rotate the bazel cache key in the secrets. It isn't really a "secret" just where this value is stored, which indicates we want a full rebuild of the bazel workspace.

uuidgen | gh secret set CACHE_VERSION -r https://github.com/googleapis/gapic-generator-php

This is documented here: https://github.com/googleapis/gapic-generator-php/blob/main/DEVELOPMENT.md#rotating-the-bazel-cache-key. You may or may not have perms for this, lmk if you don't.

I've rotated and rerun the build for you.

[noahdietz]

Please remove the MODULE bazel files - these are created by buildifier or something when you open a WORKSPACE or BUILD.bazel file. We don't support bzlmod yet.

Maybe we should add these to the .gitignore....

[Hectorhammett]

Please remove the MODULE bazel files - these are created by buildifier or something when you open a WORKSPACE or BUILD.bazel file. We don't support bzlmod yet.

Maybe we should add these to the .gitignore....

@noahdietz
I removed these. Is there a reason why NOT to add them to the .gitignore? If not I could just add them rn unless there is a specific reason not to.

[noahdietz]

I removed these. Is there a reason why NOT to add them to the .gitignore? If not I could just add them rn unless there is a specific reason not to.

Not really, unless we worry about folks forgetting that we are ignoring them if we eventually add bzlmod support haha.

Let's just add it! This PR or another is fine.

[Hectorhammett]

I removed these. Is there a reason why NOT to add them to the .gitignore? If not I could just add them rn unless there is a specific reason not to.

Not really, unless we worry about folks forgetting that we are ignoring them if we eventually add bzlmod support haha.

Let's just add it! This PR or another is fine.

@noahdietz
Let's do it in a different PR. This is getting too mangled for what it is. Just a small code change and 114 files change feels like when you have a play dough getting mixed with different colours and ends up gray 🤣