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: --build.bin path for windows #590

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kareemmahlees
Copy link
Contributor

closes #589

Changes

  • Path fed to --build.bin is converted to an absolute path always as it is done here

Any breaking change

No

Is this change easily revertible

Yes

Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 68.85%. Comparing base (9f85057) to head (350a829).
Report is 1 commits behind head on master.

Files Patch % Lines
main.go 0.00% 4 Missing ⚠️
runner/config.go 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   67.12%   68.85%   +1.72%     
==========================================
  Files          12       12              
  Lines        1080     1095      +15     
==========================================
+ Hits          725      754      +29     
+ Misses        266      250      -16     
- Partials       89       91       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kareemmahlees
Copy link
Contributor Author

I can't test the error case of WithArgs because filepath.Abs's implementation varies by platform.

if value.Value != nil && *value.Value != unsetDefault {
// To avoid path resolving problems on Windows.
if key == "build.bin" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think put logic in there is a good choice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

and we need to apply logic to windows only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think put logic in there is a good choice.

I agree on this one, I am thinking of moving the logic inside setValue2Struct, wdyt!

@xiantang xiantang self-requested a review May 28, 2024 08:31
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.

Using the air cli on Windows breaks when providing nested paths to build.bin
2 participants