JSON.sh uses bashisms #18

Merged
merged 4 commits into from Mar 12, 2013

Conversation

Projects
None yet
2 participants
@aidanhs
Collaborator

aidanhs commented Mar 12, 2013

Such as ==, let and [[. This reduces compatibility with (for example) bourne shell and dash.

(I'm working on a small PR, but am not sure if you actually want to cater for the case where someone wants to source the script instead of run it - clearly it's used in the tests, but if I tweak those to work differently are you still interested in being able to source it?)

@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Mar 11, 2013

Owner

It would be great to get it running in straight sh. But yes, I think the ability to source is a requirement.

Is it possible to get sh compatibility while maintiaining sourcability?

Owner

dominictarr commented Mar 11, 2013

It would be great to get it running in straight sh. But yes, I think the ability to source is a requirement.

Is it possible to get sh compatibility while maintiaining sourcability?

@aidanhs

This comment has been minimized.

Show comment
Hide comment
@aidanhs

aidanhs Mar 12, 2013

Collaborator

From my research it looks like it'd be really tricky to get automatic sourcing working for bourne shell. It also looks like you cannot use arguments when sourcing scripts in the bourne shell.

So my thoughts here would be that another command line option is added (say -p) which forces parsing (as the default for all shells bar bash would have to be to source). This is a bit ugly, as it means by default running in sh would do nothing. But it allows both sourcing and running in all posix compatible shells, which is something.

I'd be interested if you have any better thoughts though.

Collaborator

aidanhs commented Mar 12, 2013

From my research it looks like it'd be really tricky to get automatic sourcing working for bourne shell. It also looks like you cannot use arguments when sourcing scripts in the bourne shell.

So my thoughts here would be that another command line option is added (say -p) which forces parsing (as the default for all shells bar bash would have to be to source). This is a bit ugly, as it means by default running in sh would do nothing. But it allows both sourcing and running in all posix compatible shells, which is something.

I'd be interested if you have any better thoughts though.

@aidanhs

This comment has been minimized.

Show comment
Hide comment
@aidanhs

aidanhs Mar 12, 2013

Collaborator

Hmm, I've thought about this a bit more and I'm not keen on divergent behaviours.

I'd prefer something consistent, although it'd break backwards compatibility. My proposal would be for ./JSON.sh to do nothing by default, but perform actions based on command line options (e.g. cat jsonfile | ./JSON.sh -t -p).

It would mean getting rid of the neat little switching bash has in the name of consistency (or it could just be left in as a special bash feature).

As repo owner, up to you.

Collaborator

aidanhs commented Mar 12, 2013

Hmm, I've thought about this a bit more and I'm not keen on divergent behaviours.

I'd prefer something consistent, although it'd break backwards compatibility. My proposal would be for ./JSON.sh to do nothing by default, but perform actions based on command line options (e.g. cat jsonfile | ./JSON.sh -t -p).

It would mean getting rid of the neat little switching bash has in the name of consistency (or it could just be left in as a special bash feature).

As repo owner, up to you.

@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Mar 12, 2013

Owner

I take it that the problem is that there is no way to do this https://github.com/dominictarr/JSON.sh/blob/master/JSON.sh#L105-L108

except in bash?

Owner

dominictarr commented Mar 12, 2013

I take it that the problem is that there is no way to do this https://github.com/dominictarr/JSON.sh/blob/master/JSON.sh#L105-L108

except in bash?

@aidanhs

This comment has been minimized.

Show comment
Hide comment
@aidanhs

aidanhs Mar 12, 2013

Collaborator

Essentially, yes. If we could rely on the filename being JSON.sh, that would be ok. Sadly I cannot find any portable way to do it.

Collaborator

aidanhs commented Mar 12, 2013

Essentially, yes. If we could rely on the filename being JSON.sh, that would be ok. Sadly I cannot find any portable way to do it.

@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Mar 12, 2013

Owner

Okay -- so what if there is a file your source, and a file that's the command?

The command file just sources the source file...

Oh, wait now I remember - In bash (etc) it's rather hard to know what file you are in - I've had problems with this,
So if you symlink a file you get a different $BASH_SOURCE and it's if you have a relative file that you want to source

(spliting your code into multiple files is reasonable in most languages)

Unfortunately, I have not yet discovered a simple way to get the absolute path to the current file that work cross-platform.

It needs to work if a parent dir, or the file itself is symlinked.

This is the best I've come up with so far, http://github.com/dominictarr/whereami

That works on osx. It was much simpler on ubuntu, because readlink -e basically did everything.

Owner

dominictarr commented Mar 12, 2013

Okay -- so what if there is a file your source, and a file that's the command?

The command file just sources the source file...

Oh, wait now I remember - In bash (etc) it's rather hard to know what file you are in - I've had problems with this,
So if you symlink a file you get a different $BASH_SOURCE and it's if you have a relative file that you want to source

(spliting your code into multiple files is reasonable in most languages)

Unfortunately, I have not yet discovered a simple way to get the absolute path to the current file that work cross-platform.

It needs to work if a parent dir, or the file itself is symlinked.

This is the best I've come up with so far, http://github.com/dominictarr/whereami

That works on osx. It was much simpler on ubuntu, because readlink -e basically did everything.

@aidanhs

This comment has been minimized.

Show comment
Hide comment
@aidanhs

aidanhs Mar 12, 2013

Collaborator

I've converted this issue into a pull request. The commits remove a number of bashisms from the main program and the tests, and allow the main program to be run with non-bash shells.

The tests sadly cannot be run with a non-bash shell as JSON.sh cannot be sourced as a library without deliberately specifying the BASH_SOURCE variable (which is a workaround actually).

Regardless, it's a step forward. It gives functionality to bourne shell users (echo "[1,2,3,4]" | sh JSON.sh) and does not break any tests.

Collaborator

aidanhs commented Mar 12, 2013

I've converted this issue into a pull request. The commits remove a number of bashisms from the main program and the tests, and allow the main program to be run with non-bash shells.

The tests sadly cannot be run with a non-bash shell as JSON.sh cannot be sourced as a library without deliberately specifying the BASH_SOURCE variable (which is a workaround actually).

Regardless, it's a step forward. It gives functionality to bourne shell users (echo "[1,2,3,4]" | sh JSON.sh) and does not break any tests.

@dominictarr dominictarr merged commit 2caf844 into dominictarr:master Mar 12, 2013

@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Mar 12, 2013

Owner

merged into version 0.1.2

cheers!

Owner

dominictarr commented Mar 12, 2013

merged into version 0.1.2

cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment