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

Different folder to copy ~/.todo.cfg for macOS on arm/x86 CPU #369

Merged
merged 2 commits into from
Nov 19, 2021

Conversation

xqliu
Copy link
Contributor

@xqliu xqliu commented Nov 12, 2021

Before submitting a pull request, please make sure the following is done:

  • Fork the repository and create your branch from master.
  • If you've added code that should be tested, add tests!
  • Ensure the test suite passes.
  • Format your code with ShellCheck.
  • Include a human-readable description of what the pull request is trying to accomplish.
  • Steps for the reviewer(s) on how they can manually QA the changes.
  • Have a fixes #XX reference to the issue that this pull request fixes.

Copy link
Member

@inkarkat inkarkat left a comment

Choose a reason for hiding this comment

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

I'm not an Apple user, so I'm missing a bit of context here. It looks like the (default) install location of Homebrew has changed for Apple's new M1 processor, right? A link to Apple documentation would have been helpful here. (But then again, maybe this is common knowledge to Apple users.)

I'm also a bit puzzled by the distinction in the comments. Shouldn't it be macOS/x64 (the current spelling of Apple's OS is macOS) vs. macOS/ARM64? Apple silicon (I think that's what you've intended to write) is not a very precise term.

@xqliu
Copy link
Contributor Author

xqliu commented Nov 15, 2021

Dear, please see the follow for homebrew install directory on Mac with m1 chip

https://mac.install.guide/homebrew/index.html

Here is a screenshot:

image

For the wording, I think your suggestion is quite good. let me change them according.

Thanks for your review

@xqliu xqliu changed the title Different folder to copy ~/.todo.cfg for arm and x86 macos Different folder to copy ~/.todo.cfg for macOS on arm/x86 CPU Nov 15, 2021
Copy link
Member

@inkarkat inkarkat 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 adding the background information about the Homebrew directory change!

@karbassi
Copy link
Member

@xqliu could you work on the tests? They are failing for macOS.

https://github.com/todotxt/todo.txt-cli/runs/4216039911?check_suite_focus=true

@inkarkat
Copy link
Member

@xqliu could you work on the tests? They are failing for macOS.

This PR only updates the readme, no code; I think this is a general problem, and this PR simply is the first run where it surfaced.

The failing test is add to file without EOL; I suspect that the macOS version (or some dependencies) got updated, and now there's a different behavior of linefeed handling (somehow), and that's breaking the test.

Does GitHub automatically update the CI system images? It would be fantastic if such an update would also automatically trigger the CI, so that any compatibility issue could be detected right away and not just when the next PR is submitted. Alternatively, if there's a feed / event / notification one could subscribe to, one of us could then manually trigger a CI build.

@karbassi
Copy link
Member

This is my fault. 🙃

I should have looked at the changes and not just the CI checks.

Approved 👍🏽 and merging 🎉.

@karbassi karbassi merged commit d0075e4 into todotxt:master Nov 19, 2021
@karbassi karbassi mentioned this pull request Nov 19, 2021
7 tasks
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.

3 participants