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

Use appropriate default value for sketches-reports-source input #7

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Use appropriate default value for sketches-reports-source input #7

merged 1 commit into from
Sep 16, 2020

Conversation

per1234
Copy link
Collaborator

@per1234 per1234 commented Sep 16, 2020

In the time since it was first conceived, the use of the report file created by the arduino/compile-sketches action has been expanded from only being a way to pass memory usage change data to the arduino/report-size-deltas action to a general purpose report of data generated from the sketch compilations, including:

  • Non-deltas size data consumed by arduino/report-size-trends
  • Compilation status for each sketch/board combination
  • Compiler warning count

It's possible additional information will be added to the report over time.

For this reason, the previous default of the arduino/compile-sketches action's sketches-report-path input: size-deltas-reports was no longer appropriate and had to be changed to avoid confusion. Even though arduino/report-size-deltas only uses the deltas data from the report, and so the previous default was somewhat logical, I think it's best for the same default name to be used for the folder and workflow artifacts containing the reports consistently across all the actions that use this report.

Unfortunately, this will be a breaking change for people who have not specified the action's sketches-reports-source input in their workflow and are thus relying on the default value.

In the time since it was first conceived, the use of the report file created by the compile-sketches action has been expanded from only being a way to pass memory usage change data to the report-size-deltas action to a general purpose report of data generated from the sketch compilations, including:

- Non-deltas size data consumed by `arduino/report-size-trends`
- Compilation status for each sketch/board combination
- Compiler warning count

It's possible additional information will be added to the report over time.

For this reason, the previous default of the `arduino/compile-sketches` action's `sketches-report-path` input: `size-deltas-reports` was no longer appropriate and had to be changed to avoid confusion. Even though `arduino/report-size-deltas` only uses the deltas data from the report, and so the previous default was somewhat logical, I think it's best for the same default name to be used for the folder and workflow artifacts containing the reports consistently across all the actions that use this report.

Unfortunately, this will be a breaking change for people who have not specified the action's `sketches-reports-source` input in their workflow and are thus relying on the default value.
@per1234 per1234 added the type: enhancement Proposed improvement label Sep 16, 2020
Copy link
Collaborator

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

Although a breaking change those are sometimes necessary. As we are currently our own best action customer I agree on doing this breaking change now before the actions get widespread usage. Good work @per1234 🚀

@aentinger aentinger merged commit 1566263 into arduino:main Sep 16, 2020
@per1234 per1234 deleted the fix-default-value branch September 16, 2020 06:23
@per1234 per1234 added the topic: code Related to content of the project itself label Jun 13, 2024
@per1234 per1234 self-assigned this Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants