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

[draft] open on different OS #58

Closed
wants to merge 3 commits into from
Closed

Conversation

rmNULL
Copy link
Contributor

@rmNULL rmNULL commented Apr 4, 2018

Fixes

Fixes #57
Closes #57

Checklist

  • I have made a change to this repository, be it functionality, testing, documentation, spelling, or grammar.

  • I updated my branch with the master branch.

  • I have added the necessary testing to prove my fix is effective or my feature work, or I did not modify functionality.

  • I have added necessary documentation about the functionality in an appropriate .md file

  • I have commented any code I have modified or I did not modify any code

  • I would prefer the approach of defining open() based on the OS

PR summary

not implemented yet. In here for discussion

Copy link
Owner

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

Good core logic, but I'd like to tweak the structure a little.

Also the tests are failing. You'll need to update the stubbing here:

function open() { echo "Opening $*"; }
export -f open

I think changing that to xdg-open and also stubbing $OS to Linux is probably the way to go.

notes Outdated
OS=`uname -s`
[[ $OS = "Linux" ]] && open() { xdg-open "$@"; }
# Windows versions have NT in them
[[ $OS = *NT* ]] && open() { false; } # how do I invoke windows commands ?
Copy link
Owner

Choose a reason for hiding this comment

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

I think:

  • We should do this inside open_note, rather than here. As-is, we'll run osname -s any time you run any command, which is just a bit wasteful, and it's not clear what the code in open_note is really doing now (it looks like it actually calls open, but it's really using this logic instead)
  • We should explicitly handle OSX here, which really does use open (that was what this code originally targeted). Looks like you're essentially handling that as a fallback case? Nicer to be clearer.
  • For versions where we don't know what to do (Windows), we should print a nice message and exit 1, with something like:
    Opening your notes directory is not supported on your platform ($OS).
    Do you know how we can support this? Open an issue! https://github.com/pimterry/notes/issues/new
    

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, we should stuff the checks inside open_note(), The current approach was a mistake, I should have checked where open() was being called before wrapping. (Note: I checked it now :)
As for the test cases, I'll have to look, why they are failing and what those test cases are for in the first place. 😅

Thanks for the feedback :)

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.

'notes o' says it can't get a file descriptor on Linux
2 participants