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.

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

Jump to bottom

Fix #303 #305

Merged
merged 3 commits into from
Apr 19, 2016
Merged

Fix #303 #305

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b3a258
Fix #303
Apr 19, 2016
a5654cf
Use -L instead of -h
Apr 19, 2016
1853f30
Use local variables
Apr 19, 2016
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: 6 additions & 1 deletion pkg/omf/functions/bundle/omf.bundle.add.fish
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
function omf.bundle.add -a type name_or_url
set -l bundle $OMF_CONFIG/bundle
if test -h $OMF_CONFIG/bundle
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm looking both at the fish website and the man page, and -h is undocumented. Maybe -L would make it easier for people to understand what it's doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

By removing the -l flag, you allow $bundle to clutter the user's environment.
I suggest keeping this line as default, and then you can change it inside the first if case, only if it's a symlink. The second case is then irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-l was not used in omf.bundle.remove.fish. Would you like the -l flag there as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please.
It's always good to have a new pair of eyes looking at the code. :)

set bundle (readlink $OMF_CONFIG/bundle)
else
set bundle $OMF_CONFIG/bundle
end

set -l record "$type $name_or_url"

if test -f $bundle
Expand Down
6 changes: 5 additions & 1 deletion pkg/omf/functions/bundle/omf.bundle.remove.fish
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
function omf.bundle.remove
set bundle $OMF_CONFIG/bundle
if test -h $OMF_CONFIG/bundle
set bundle (readlink $OMF_CONFIG/bundle)
else
set bundle $OMF_CONFIG/bundle
end

if test -f $bundle
set type $argv[1]
Expand Down