Skip to content

[draft] open on different OS #58

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.

Sign up for GitHub

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

Closed
rmNULL wants to merge 3 commits into from
Closed

[draft] open on different OS #58

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b408632
[draft]testing open on different OS
Apr 3, 2018
cd00671
Merge remote-tracking branch 'upstream/master'
Apr 9, 2018
2d24fa4
file manager handling
Apr 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions notes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ if [ -z "$EDITOR" ] && type editor &>/dev/null; then
EDITOR=editor
fi

# reference, https://en.wikipedia.org/wiki/Uname
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

@pimterry pimterry Apr 7, 2018

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

@rmNULL rmNULL Apr 7, 2018

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 :)



without_notes_dir() {
cat | sed -e "s/^$escaped_notes_dir//g" | sed -E "s/^\/+//g"
}
Expand Down