Command Plugin for generating the source code #98

MahdiBM · 2023-07-02T06:38:39Z

Motivation

This PR adds the option to use the package as a Command plugin instead of a BuildTool plugin.

This benefits those who use heavy OpenAPI documents, and prefer not to have to wait for an extra round of OpenAPI code generation which can be accidentally triggered at times, for example if you clean your build folder.

The whole idea of creating this Command plugin came after @czechboy0 's comment here: #96 (comment)

Modifications

Generally, add a Command plugin target to the package, plus modifying the functions etc... to match/allow this addition.

Result

There is a new Command plugin, and users can choose between the Command plugin and the BuildTool plugin at will.

Test Plan

As visible in the PR discussions below, we've done enough manual-testing of the Command plugin.

Plugins/OpenAPIGeneratorCommandPlugin/plugin.swift

simonjbeaumont · 2023-07-20T08:52:10Z

This seems good to me. To summarise:

If you don't select any target, it tries them all and "skips" targets that aren't deemed appropriate.
If you explicitly select an appropriate target, it succeeds.
If you explicitly select an inappropriate target, it fails.

In scenario (1), I can see in the screenshot two things that would be nice to improve:

The order of the logs: would be nice to log the target being operated on before the rest of the logging. This reads clearer IMO.
Would be nice for these "skipped" targets to appear as "skipped" or "warnings" rather than "failed" in the logs.

I don't mind deferring that to another PR though. WDYT @czechboy0 @MahdiBM?

MahdiBM · 2023-07-20T09:04:46Z

@simonjbeaumont
I know this PR has been going for long enough that you probably don't want to bother me anymore, but those changes are small enough, and even if they weren't, i still wouldn't mind them 😅

The order of the logs: would be nice to log the target being operated on before the rest of the logging. This reads clearer IMO.

Good idea but will basically duplicate the logs because we'll need to say running on target '----' then run failed on target '-----'. (Unless the target is just straight up not a swift source module in which case no need for duplication.)

Should i go for it? or did you mean something else?

Would be nice for these "skipped" targets to appear as "skipped" or "warnings" rather than "failed" in the logs.

No objections, but can you mention some examples so i know what you exactly have in mind?
Like just changing the words, or kind of flagging the print statements by starting them with e.g. Skipped target '----':, Warnings for target '-----':?
EDIT: Ah reading your message again, you probably just meant changing the word. Though i still like the other idea 🤔

MahdiBM · 2023-07-20T09:09:02Z

What i do prefer though, is for the Docs part to be deferred to another PR considering this has taken enough time/length 😅 would prefer this to be merged sooner than later, when possible, so i can also start using the main Apple repo in my Package.swifts 🙂

simonjbeaumont · 2023-07-20T09:35:42Z

@simonjbeaumont I know this PR has been going for long enough that you probably don't want to bother me anymore, but those changes are small enough, and even if they weren't, i still wouldn't mind them 😅

OK, great!

Both of these comments pertain to this logging from your screenshot above, so I'll paste the textual version here before continuing to discuss your specific questions:

✅ Invoking plug-in command "OpenAPIGeneratorCommand" 18.5 seconds
- Target 'PennyTests': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'Models': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'Penny': OpenAPI code generation failed. No expected config or OpenAPI document files.
Swift OpenAPI Generator is running with the following configuration:
- OpenAPI document path: /Users/mahdibm/Github/penny-bot/Sources/GitHubAP|/openapi.yaml
- Configuration path: /Users/mahdibm/Github/penny-bot/Sources/GitHubAP|/openapi-generator-config.yml
- Generator modes: types, client
- Output file names: Types.swift, Client.swift
- Output directory: /Users/mahdibm/Github/penny-bot/Sources/GitHubAP|/GeneratedSources
- Diagnostics output path: <none - logs to stderr>
- Current directory: /Users/mahdibm/Github/penny-bot
- Plugin source: command
- Additional imports: <none>
File Types.swift: unchanged
File Client.swift: unchanged
- Target 'GitHubAPI': OpenAPI code generation successfully completed.
- Target 'Extensions': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'GHHooksLambda': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'Fake': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'SponsorsLambda': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'SharedServices': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'CoinsLambda': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'FaqsLambda': OpenAPI code generation failed. No expected config or OpenAPI document files.
- Target 'AutoPingsLambda': OpenAPI code generation failed. No expected config or OpenAPI document files.
✅ Activity Log Complete 7/20/23, 10:50 AM 17.8 seconds
No issues

The order of the logs: would be nice to log the target being operated on before the rest of the logging. This reads clearer IMO.

Good idea but will basically duplicate the logs because we'll need to say running on target '----' then run failed on target '-----'. (Unless the target is just straight up not a swift source module in which case no need for duplication.)

Should i go for it? or did you mean something else?

Personally, I prefer logging to be before an operation, and, looking through the build logs in Xcode for a target I happen to have open I see this pattern. Examples include Compiling ..., and Discovering Swift tasks after .... When an operation is failed, there is some "duplicate" logging; here is a textual version of what I see in the report navigator when I replace print with pint (note, the emojis aren't actually in the log—they are a representation of how Xcode renders the icons):

...
✅ Planning Swift module swift_openapi_generator (arm64)
ℹ️ Compiling runGenerator.swift 
    ℹ️ Compile runGenerator.swift (arm64)
        ❌ Cannot find 'pint' in scope
❌ Build failed ...

When the issue if fixed, I see:

✅ Planning Swift module swift_openapi_generator (arm64)
✅ Compiling runGenerator.swift 
    ✅ Compile runGenerator.swift (arm64)
...
✅ Build succeeded ...

Would be nice for these "skipped" targets to appear as "skipped" or "warnings" rather than "failed" in the logs.

No objections, but can you mention some examples so i know what you exactly have in mind? Like just changing the words, or kind of flagging the print statement by starting them with e.g. Skipped target '----':, Warnings for target '-----':?

I think it's odd to see big green check marks and lots of <some operation> failed in the logs.

I'm not suggesting we go for emojis, but I'd be happy with us finding some terminology that is clear about what's going on, for example:

Considering target for OpenAPI generation: InappropriateTarget
Skipped target (no OpenAPI document): InappropriateTarget
Considering target for OpenAPI generation: AppropriateTarget
Running Swift OpenAPI Generator on target AppropriateTarget with the following configuration:
...

I'd be happy if it then printed a line explicitly saying whether it was succeeded or failed.

IMHO this small amount of "duplicate" logging is a small price to pay for clarity when debugging a failed build.

MahdiBM · 2023-07-20T13:15:54Z

@simonjbeaumont This is what i came up with.

Please let me know if i can improve any of the sentences.

I used the ✅ emoji for a successful code generation because i think it makes long logs like this more skimmable.

The - Stopping because target isn't configured for OpenAPI code generation. logs don't feel satisfying in terms of being different from the theme/mood of the rest of the logs, but that's the best i could come up with.

simonjbeaumont · 2023-07-21T13:11:09Z

@simonjbeaumont This is what i came up with.
Thanks for going the extra mile on this one.

I used the ✅ emoji for a successful code generation because i think it makes long logs like this more skimmable.

While I initially hinted at not doing this, I think I agree; it does make it easier to read at a glance so I'm fine with it 👍

Please let me know if i can improve any of the sentences.

The - Stopping because target isn't configured for OpenAPI code generation. logs don't feel satisfying in terms of being different from the theme/mood of the rest of the logs, but that's the best i could come up with.

Aside from tone, this one in particular isn't informative—why isn't it "configured" and what does it mean for it to be configured? I this case we could have something like: - Skipped target: Target does not contain OpenAPI document or something similar?

Other than that I'm pleased with how this is looking.

@czechboy0 do you have anything to add before we push this over the line?

MahdiBM · 2023-07-21T13:18:38Z

@simonjbeaumont i made it non-informative because i suspected @czechboy0 would not like the duplication of "there are no files" messages as he requested some changes about them in the past reviews.

Though i think both views are completely valid. It's just a question of if the information is worth the duplication. I'm personally fine with both, maybe slightly leaning towards the current non-informative message.

simonjbeaumont · 2023-07-21T13:21:34Z

@simonjbeaumont i made it non-informative because i suspected @czechboy0 would not like the duplication of "there are no files" messages as he requested some changes about them in the past reviews.

Though i think both views are completely valid. It's just a question of if the information is worth the duplication. I'm personally fine with both, maybe slightly leaning towards the current non-informative message.

OK, that's fair enough, and sorry for missing that historical review context.

I'm happy with things the way they are now (subject to the soundness check passing again).

MahdiBM · 2023-07-21T13:38:48Z

Honestly i had no idea what some of the warnings were requesting:

** ✅ Found no unacceptable language.
** Running /code/scripts/check-license-headers.sh...
** ERROR: Unsupported file extension for file (exclude or update this script): Plugins/OpenAPIGenerator/PluginsShared
** Running /code/scripts/run-swift-format.sh...
Plugins/PluginsShared/PluginUtils.swift:4:92: warning: [UseEarlyExits] replace the `if/else` block with a `guard` statement containing the early exit
Plugins/PluginsShared/PluginUtils.swift:4:92: warning: [UseEarlyExits] replace the `if/else` block with a `guard` statement containing the early exit
/code/Package.swift:173:10: warning: [TrailingComma] add trailing comma to the last element in multiline collection literal
Package.swift:164:1: warning: [LineLength] line is too long
Plugins/PluginsShared/PluginError.swift:14:1: warning: [LineLength] line is too long
Plugins/PluginsShared/PluginError.swift:21:1: warning: [LineLength] line is too long
Plugins/PluginsShared/PluginError.swift:97:1: warning: [LineLength] line is too long
Plugins/PluginsShared/PluginError.swift:99:1: warning: [LineLength] line is too long
Plugins/PluginsShared/PluginError.swift:104:1: warning: [LineLength] line is too long
Plugins/PluginsShared/PluginError.swift:106:1: warning: [LineLength] line is too long
Plugins/PluginsShared/PluginUtils.swift:4:1: warning: [LineLength] line is too long
Plugins/OpenAPIGeneratorCommand/plugin.swift:58:1: warning: [LineLength] line is too long
Plugins/OpenAPIGeneratorCommand/plugin.swift:59:1: warning: [LineLength] line is too long
Plugins/OpenAPIGeneratorCommand/plugin.swift:62:1: warning: [LineLength] line is too long
** ERROR: ❌ Running swift-format produced errors.

  To fix, run the following command:

    % git ls-files -z '*.swift' | xargs -0 swift-format --in-place --parallel

A few of the warnings were also pointing at non-existent (or wrong) lines (like Package.swift being 172 lines and swift-format complaining about the 173 rd line!)
But luckily i noticed the CI also mentions git ls-files -z '*.swift' | xargs -0 swift-format --in-place --parallel which automatically solved the problems.

EDIT: Not to mention the line is too long warnings which were in short complaining about something that couldn't have been solved, and even the command didn't really fix anything, it just moved the long lines to the line after the returns which is the best it could do.

MahdiBM · 2023-07-21T13:41:37Z

@simonjbeaumont @czechboy0 what should i do to resolve this last soundness error?


** ✅ Found no unacceptable language.
** Running /code/scripts/check-license-headers.sh...
** ERROR: Unsupported file extension for file (exclude or update this script): Plugins/OpenAPIGenerator/PluginsShared
** Running /code/scripts/run-swift-format.sh...
** ✅ Ran swift-format with no errors.

MahdiBM · 2023-07-21T14:09:32Z

Had to exclude 2 to directories from check-license-headers.sh.
These are the symlinked directories, and the actual files are still checked for headers, as it threw errors for the files that didn't have the headers in the second to last CI run.

read -ra PATHS_TO_CHECK_FOR_LICENSE <<< "$( \
  git -C "${REPO_ROOT}" ls-files -z \
  ":(exclude).gitignore" \
  ":(exclude).spi.yml" \
  ":(exclude).swift-format" \
  ":(exclude).github/*" \
  ":(exclude)CODE_OF_CONDUCT.md" \
  ":(exclude)CONTRIBUTING.md" \
  ":(exclude)CONTRIBUTORS.txt" \
  ":(exclude)LICENSE.txt" \
  ":(exclude)NOTICE.txt" \
  ":(exclude)Package.swift" \
  ":(exclude)Package.resolved" \
  ":(exclude)README.md" \
  ":(exclude)SECURITY.md" \
  ":(exclude)scripts/unacceptable-language.txt" \
  ":(exclude)Tests/PetstoreConsumerTests/Generated" \
  ":(exclude)Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/*" \
  ":(exclude)docker/*" \
  ":(exclude)**/*.docc/*" \
  ":(exclude)**/.gitignore" \
  ":(exclude)**/Package.swift" \
  ":(exclude)**/Package.resolved" \
  ":(exclude)**/README.md" \
  ":(exclude)**/openapi.yaml" \
  ":(exclude)**/openapi.yml" \
  ":(exclude)**/petstore.yaml" \
  ":(exclude)**/openapi-generator-config.yaml" \
  ":(exclude)**/openapi-generator-config.yml" \
+  ":(exclude)Plugins/OpenAPIGenerator/PluginsShared" \
+  ":(exclude)Plugins/OpenAPIGeneratorCommand/PluginsShared" \
  | xargs -0 \
)"

czechboy0 · 2023-07-21T17:18:57Z

Thanks @MahdiBM for bearing with us during this marathon of a PR 🙂 Just updated from main and will land once CI passes.

MahdiBM added 19 commits July 2, 2023 05:16

try generator plugin

31cf1d5

add new plugin to products

c5a404d

better debugging

cfb128d

Update plugin.swift

b6e4ffe

fixes

7fbde61

correct working dir

d65e0bf

correct file-manipulation logic

45c8824

better file manipulation

133a3c1

generate empty files only for buildtool plugins

12fec26

change directory name

db96114

perform cleanup for previous builds

7fd39a2

better cleanup

fbd69d1

refinements

d6005dc

Update plugin.swift

02cda11

fixes for same-name file creations by plugins

7f7b33a

better configuration

11ee904

fix arguments

0198e6c

better cleanups

7ada532

better cleanups

5460188

Kyle-Ye reviewed Jul 2, 2023

View reviewed changes

Plugins/OpenAPIGeneratorCommandPlugin/plugin.swift Outdated Show resolved Hide resolved

MahdiBM added 10 commits July 3, 2023 00:13

move plugins shared logic to shared files

3cb098d

rename and fix 'invocation-kind' arg to 'invoked-from'

42c73f3

InvocationSource -> internal

253e17f

better (or worse) InvocationSource handling

31365ed

remove a lot of unneeded complexity

617edfe

better message when running the generator

c381a0c

cleanup

e2c085c

fix passing bad arguments

ff25cba

even more removing unneeded complexity

ceaeb2d

refinements

9887742

MahdiBM added 4 commits July 20, 2023 16:05

apply logging suggestions

8fb9775

Update plugin.swift

e9be1e7

try ✅ in logs

29b6301

Update plugin.swift

d8e5a47

remove extra space

1b5286f

simonjbeaumont approved these changes Jul 21, 2023

View reviewed changes

czechboy0 approved these changes Jul 21, 2023

View reviewed changes

swift-format to the rescue of me from itself!

7c9cee3

MahdiBM added 5 commits July 21, 2023 17:17

exclude directory

d4b7756

another try to exclude directory from headers-check script

a2b0ab4

headers script another try

96398f4

fix the script itself (the check should still fail)

a4f8584

final soundness fix hopefully?

4b4d0f8

Merge branch 'main' into generator-command-plugin

a611ae2

czechboy0 enabled auto-merge (squash) July 21, 2023 17:19

czechboy0 merged commit 4198dc6 into apple:main Jul 21, 2023

czechboy0 mentioned this pull request Jul 21, 2023

Add dry run option to CLI #123

Merged

czechboy0 added the 🔨 semver/patch No public API change. label Jul 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command Plugin for generating the source code #98

Command Plugin for generating the source code #98

MahdiBM commented Jul 2, 2023 •

edited

Loading

simonjbeaumont commented Jul 20, 2023

MahdiBM commented Jul 20, 2023 •

edited

Loading

MahdiBM commented Jul 20, 2023

simonjbeaumont commented Jul 20, 2023

MahdiBM commented Jul 20, 2023

simonjbeaumont commented Jul 21, 2023

MahdiBM commented Jul 21, 2023

simonjbeaumont commented Jul 21, 2023

MahdiBM commented Jul 21, 2023 •

edited

Loading

MahdiBM commented Jul 21, 2023

MahdiBM commented Jul 21, 2023

czechboy0 commented Jul 21, 2023

Command Plugin for generating the source code #98

Command Plugin for generating the source code #98

Conversation

MahdiBM commented Jul 2, 2023 • edited Loading

Motivation

Modifications

Result

Test Plan

simonjbeaumont commented Jul 20, 2023

MahdiBM commented Jul 20, 2023 • edited Loading

MahdiBM commented Jul 20, 2023

simonjbeaumont commented Jul 20, 2023

MahdiBM commented Jul 20, 2023

simonjbeaumont commented Jul 21, 2023

MahdiBM commented Jul 21, 2023

simonjbeaumont commented Jul 21, 2023

MahdiBM commented Jul 21, 2023 • edited Loading

MahdiBM commented Jul 21, 2023

MahdiBM commented Jul 21, 2023

czechboy0 commented Jul 21, 2023

MahdiBM commented Jul 2, 2023 •

edited

Loading

MahdiBM commented Jul 20, 2023 •

edited

Loading

MahdiBM commented Jul 21, 2023 •

edited

Loading