Skip to content

Issue #8, convert hunspell output to spellchecker JSON format - #9

Open
teners wants to merge 22 commits into
masterfrom
tkt_8_convert_hunspell_to_json
Open

Issue #8, convert hunspell output to spellchecker JSON format#9
teners wants to merge 22 commits into
masterfrom
tkt_8_convert_hunspell_to_json

Conversation

@teners

@teners teners commented Aug 10, 2016

Copy link
Copy Markdown

No description provided.

@teners teners self-assigned this Aug 10, 2016
Comment thread backend/spellchecker/huntojson.sh Outdated
done
shift $((OPTIND-1))

if [ "$#" != 1 ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any sense to surround 1 with double-quotes, to compare string and string?

fixed hunspell options to interpret input as latex file
Comment thread backend/spellchecker/huntojson.sh Outdated
INFILE=$1

JSON=$(
cat $INFILE |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Enquote $INFILE, otherwise you're going have troubles with spaces in the path

@dbarashev

Copy link
Copy Markdown
Contributor

One interesting question is duplicate keys:
echo "documeent documeent documeent" | hunspell -a

This script will produce JSON with three equals keys. I suggest filtering duplicates with | jq '.'

Comment thread backend/spellchecker/huntojson.sh Outdated
options_number=split(split_string[2], options, ", ");

for (i = 1; i <= options_number; i++)
{print "\t\t\""options[i]"\","}

@dbarashev dbarashev Aug 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comma after the last element of the array produces invalid JSON, at least from jq perspective.The same applies to the commma after the last entry

root and others added 2 commits August 11, 2016 16:34
fixed it with an additional variable in awk
Comment thread backend/spellchecker/huntojson.sh Outdated

echo $JSON
echo "$JSON" |
sed ':a;N;$!ba;s/,\n}/\n}/g'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can probably be better solved by introducing a global variable which is empty in the beginning and "," after the first use. Use it like a prefix of the serialized array:

print $2 + PREFIX + [

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Heck, that's a really good point. Shame on me for such a dummy solution.

}

message Suggestions {
string json = 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one of the purposes of protocol buffers is to provide typed interface for data exchange between servers. Please replace this json with typed interface, like map<string, Replacements> suggestions, use it in the spellchecker and remove your own json-serialization

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Makes a lot of sense, right.

Comment thread backend/spellchecker/server/parser.py Outdated
# note: these fields' names doesn't start with "_" since deleting of this
# object isn't so straightforward -- for some reason interpreter destroys
# these "private" fields before __del__ is invoked.
self.parser_lib_ = cdll.LoadLibrary("../libparser/build/libparser.so")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what's the purpose of the trailing underscores?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also, please parameterize this class with the path to the shared library

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh, I thought I removed them.

Yep, that's what I forgot to do.

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.

2 participants