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

Fix changed files check for APIs in CI #2389

Merged
merged 8 commits into from
Nov 25, 2024
Merged

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Nov 25, 2024

There were two issues:

  1. We unintentionally ignored API docs because --from-file was being combined with the other args.
  2. Our call to echo the changed files to save them could blow up the overall GitHub Action when there were too many files. Instead, we can use the changed-files action to write the output directly.

@Eric-Arellano Eric-Arellano changed the title [wip] Fix changed files check when PR has lots of files [wip] Fix changed files check for APIs in CI Nov 25, 2024
@Eric-Arellano Eric-Arellano changed the title [wip] Fix changed files check for APIs in CI Fix changed files check for APIs in CI Nov 25, 2024
@Eric-Arellano
Copy link
Collaborator Author

@Eric-Arellano Eric-Arellano marked this pull request as ready for review November 25, 2024 20:20
Copy link
Collaborator

@arnaucasau arnaucasau left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you!

"Read the file path for file paths and globs to check, like `docs/start/index.md`. Entries should be separated by a newline and should be valid file types (.md, .mdx, .ipynb).",
"Read the file path for file paths and globs to check, like `docs/start/index.md`. " +
"Entries should be separated by a newline and should be valid file types (.md, .mdx, .ipynb)." +
"Mutually exclusive with the other args.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice comment! Maybe in the future, we can refactor it and use .conflcts() to error if --from-file is used with another argument to avoid setting them unintentionally.

https://yargs.js.org/docs/#api-reference-conflictsx-y

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, in 5b57701, we also need to remove the default: false from the other arguments and let them be undefined. Otherwise, they count as set by conflicts and the option from-files will always error

Co-authored-by: Arnau Casau <[email protected]>
@Eric-Arellano Eric-Arellano added this pull request to the merge queue Nov 25, 2024
Merged via the queue into main with commit b62d46d Nov 25, 2024
3 checks passed
@Eric-Arellano Eric-Arellano deleted the EA/fix-changed-files branch November 25, 2024 21:19
github-merge-queue bot pushed a commit that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants