Skip to content

Format codegen files #1301

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

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Format codegen files #1301

merged 2 commits into from
Feb 2, 2023

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Feb 1, 2023

Codegen was not formatted. IMO we should do that to keep the code consistent

@kimdv kimdv requested a review from ahoppen as a code owner February 1, 2023 10:06
@kimdv
Copy link
Contributor Author

kimdv commented Feb 1, 2023

This should be coordinated with some of the other PR's before merge

@ahoppen
Copy link
Member

ahoppen commented Feb 1, 2023

I deliberately decided not to format the generated files because it simplifies development: If you make changes to CodeGenerateion and re-generate the files, you aren’t hit with a wall of changes because now the files are no longer formatter.

If we want to format those files (which I think is a good idea), IMO the formatting should be done in CodeGeneration itself at generation time. That way running CodeGeneration will always generate formatted files. For this, CodeGeneration can depend on swift-format, similarly to how it depends on swift-syntax.

@kimdv
Copy link
Contributor Author

kimdv commented Feb 1, 2023

I don't format the generated files, but the code generation project 😅

Like template files etc.
Based on this comment:
#1293 (comment)

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Oh, I should have looked at the PR more closely, sorry. CodeGeneration itself should certainly be formatted by swift-format. Thanks for catching and fixing this. I wonder whether we can generalize this a little further to also include the examples (see inline comment).

@kimdv kimdv force-pushed the kimdv/format-codegen-file branch from ea81280 to 4f2ab61 Compare February 1, 2023 20:04
@kimdv
Copy link
Contributor Author

kimdv commented Feb 1, 2023

I tried to include all relevant files as you sugested.
With an if statement is exploded in cases, so I tought I did find all the files to exclude and then filter at the end

@kimdv kimdv force-pushed the kimdv/format-codegen-file branch from 69f1525 to 021131a Compare February 2, 2023 07:13
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks for catching that we didn’t format all of these files and fixing it.

# Don't format .build folder and content
files_to_exclude.update(package_dir.glob('**/.build/**/*.swift'))
# Don't format test input files
files_to_exclude.update(package_dir.glob('**/Inputs/**/*.swift'))
Copy link
Member

Choose a reason for hiding this comment

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

This looks really nice 😍

@kimdv
Copy link
Contributor Author

kimdv commented Feb 2, 2023

@swift-ci plese test

@kimdv
Copy link
Contributor Author

kimdv commented Feb 2, 2023

@swift-ci please test

@kimdv kimdv merged commit c47db5b into swiftlang:main Feb 2, 2023
@kimdv kimdv deleted the kimdv/format-codegen-file branch February 9, 2023 14:19
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.

2 participants