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: add support for .zip files in binary download #417

Merged

Conversation

lumirlumir
Copy link
Contributor

@lumirlumir lumirlumir commented Jan 9, 2025

What changes this PR introduce?

Hello, @theoludwig

This PR resolves the issue metioned in editorconfig-checker/editorconfig-checker#415.

I wrote the code in a way that doesn't affect the existing behavior.

List any relevant issue numbers

editorconfig-checker/editorconfig-checker#415

Is there anything you'd like reviewers to focus on?

I used the admzip library to unzip .zip files. It’s a well-maintained and organized package, which is why I chose it. link

I added logic to detect .zip files and unzip them in the same way we handle .tar.gz.

Result

When I set indent_size = 3 in .editorconfig and run it, I can confirm it works as expected.

image

image

Environment

This is my current env.

Operating System:
  Platform: win32
  Arch: x64
  Version: Windows 11 Home
  Available memory (MB): 16058
  Available CPU cores: 8
Binaries:
  Node: 20.18.0
  npm: N/A
  Yarn: N/A
  pnpm: N/A

If there’s anything wrong, please let me know.

theoludwig
theoludwig previously approved these changes Jan 9, 2025
Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

Thank you! 😄

@theoludwig
Copy link
Member

theoludwig commented Jan 9, 2025

Not blocking, but could you please try to merge my PR #418 on your branch, to see if it resolves the issue with CI, and to check if it works on all Operating Systems?

@theoludwig theoludwig requested a review from klaernie January 9, 2025 14:26
@lumirlumir
Copy link
Contributor Author

Sure, I merged it!

@lumirlumir lumirlumir requested a review from theoludwig January 9, 2025 14:40
@lumirlumir
Copy link
Contributor Author

lumirlumir commented Jan 9, 2025

I think the ci script of commitlint should be different in Windows. (or also for other scripts)

E.g. POSIX uses bash while Windows uses Powershell or cmd.

- run: your script
  if: runner.os != 'Windows'

- run: your script
  if: runner.os == 'Windows'

@theoludwig
Copy link
Member

I think the ci script of commitlint should be different in Windows. (or also for other scripts)

E.g. POSIX uses bash while Windows uses Powershell or cmd.

- run: your script
  if: runner.os != 'Windows'

- run: your script
  if: runner.os == 'Windows'

Yes, I've updated #418 can you please merge back again? 😄

@lumirlumir
Copy link
Contributor Author

Yep!

@lumirlumir
Copy link
Contributor Author

lumirlumir commented Jan 9, 2025

image

I also think that the "start": "./dist/index.js", script should be fixed like "start": "node ./dist/index.js", or "start": "node dist"?

. command would be OS specific. (for POSIX)


I've added a commit about it.

@theoludwig
Copy link
Member

Indeed.

On a side note, GitHub rate limiting is annoying.
We'll execute it a bit later, it should work after a while.

@lumirlumir
Copy link
Contributor Author

@theoludwig

I agree. If the GitHub rate limit is lifted, the tests will pass.

@theoludwig
Copy link
Member

@theoludwig

I agree. If the GitHub rate limit is lifted, the tests will pass.

I believe, we should also add fail-fast: false in the strategy section so that it doesn't fail everything, might help a bit.

@lumirlumir
Copy link
Contributor Author

lumirlumir commented Jan 9, 2025

@theoludwig
I agree. If the GitHub rate limit is lifted, the tests will pass.

I believe, we should also add fail-fast: false in the strategy section so that it doesn't fail everything, might help a bit.

Added fail-fast: false 👍

@lumirlumir
Copy link
Contributor Author

lumirlumir commented Jan 9, 2025

Let's add .gitattributes file to ensure LF for all Git environment.

I think Windows container turns LF to CRLF?

.gitattributes

# https://git-scm.com/docs/gitattributes

# Ensure consistent EOL(LF) for all files that Git considers text files.
* text=auto eol=lf

image

@lumirlumir
Copy link
Contributor Author

Coooooool. We did it.🔥

@theoludwig
Copy link
Member

@klaernie
Let's merge, as it resolves a critical bug/issue. I bypass the 72h here, I guess it's fine, else we are able to revert if there are objections.
And CI is happy, seems to work on all Operating systems.

@theoludwig theoludwig merged commit f029f8e into editorconfig-checker:master Jan 9, 2025
5 checks passed
Copy link

github-actions bot commented Jan 9, 2025

🎉 This PR is included in version 6.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lumirlumir lumirlumir deleted the fix-add-support-for-zip branch January 9, 2025 15:55
@theoludwig
Copy link
Member

theoludwig commented Jan 9, 2025

Coooooool. We did it.🔥

It doesn't seems to work on the main branch: https://github.com/editorconfig-checker/editorconfig-checker.javascript/actions/runs/12693264299/job/35388685716

Already 6 run. And always on macOS, it seems like.

Ah... I give up. 😆 We know it's working so that's not critical at least.

I created an issue about it: #419

@xsy420
Copy link

xsy420 commented Jan 9, 2025

it just because github rate limit, maybe just run it again? or create a new github token and add it into request header?

@theoludwig
Copy link
Member

it just because github rate limit, maybe just run it again?

I tried it 6 times. It's just based on luck, really.

I just re ran it a 7th time, and it seemed to work lol => https://github.com/editorconfig-checker/editorconfig-checker.javascript/actions/runs/12693264299/job/35393273587

or create a new github token and add it into request header?

Yes, that could be a fix, indeed. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants