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

Improving Javascript style #41

Merged
merged 2 commits into from
Mar 1, 2015
Merged

Improving Javascript style #41

merged 2 commits into from
Mar 1, 2015

Conversation

vandanphadke
Copy link
Contributor

As per our conversation I am sending this PR.
Please guide me if I am not going in the right direction.


/**
* @desc Constructs google translate url
* @params User saved preferences
Copy link
Contributor

Choose a reason for hiding this comment

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

From google's style guide:

/**
 * Some class, initialized with a value.
 * @param {Object} value Some value.
 * @constructor
 */
function MyClass(value) {
  /**
   * Some value.
   * @type {Object}
   * @private
   */
  this.myValue_ = value;
}

ceilican added a commit that referenced this pull request Mar 1, 2015
@ceilican ceilican merged commit 038e6c0 into OiWorld:master Mar 1, 2015
@ceilican
Copy link
Contributor

ceilican commented Mar 1, 2015

Thanks for the pull request, Vandan! I have just merged it.

You are going in the right direction, but a few things are not according to Google's style guidelines for Javascript (https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml). For example, 2 spaces should be used for code block indentation, while 4 spaces should be used for statement break indentation. Have a look on the style guide, and you will see many examples of good style there.

@ceilican
Copy link
Contributor

ceilican commented Mar 1, 2015

Also, follow git's convention and use imperative instead of gerund in your commit messages. For example, among git users, it would be generally considered better to write "Improve Javascript style" than "Improving Javascript style".

@vandanphadke
Copy link
Contributor Author

Thanks a lot ...
I will take these things into consideration and send the next pull request.

I had a some small doubts in mind

And what is the function of the ngrammin and ngramMax params in getAllWords
method?
Is POS tagging already integrated ?
How exactly are we selecting which words to translate and in which method
are we filtering it ?

Vandan Phadke
On 1 Mar 2015 16:58, "Bruno" [email protected] wrote:

Also, follow git's convention and use imperative instead of gerund in your
commit messages. For example, among git users, it would be generally
considered better to write "Improve Javascript style" than "Improving
Javascript style".


Reply to this email directly or view it on GitHub
#41 (comment).

@vandanphadke vandanphadke deleted the jsStyle branch March 1, 2015 13:51
@ceilican
Copy link
Contributor

ceilican commented Mar 1, 2015

By default, MindTheWord translates isolated words. However, there is an advanced option in the options page that allows the user to choose ngram-min and ngram-max. When the user selects, for example, ngram-min = 2 and ngram-max = 4, MindTheWord will translate sequences of consecutive words containing from 2 to 4 words.

@ceilican
Copy link
Contributor

ceilican commented Mar 1, 2015

POS tagging is currently not used. If we had it, we could easily implement the following requested feature: #30

The difficulty are:

  1. POS tagging tools are probably not available for all languages.
  2. relying on a web-service that provides POS tagging would probably be too slow.

If you have any idea how how to overcome these challenges in a simple way, I would really like to hear it!

@ceilican
Copy link
Contributor

ceilican commented Mar 1, 2015

The text is broken into a collection of words (or n-grams/wordsequences) in the method getAllWords. Perhaps "getAllWordSequences" would be a better name for this method.

Then the words are filtered in filterSourceWords. Issue 37 (#37) seems to be affecting this method. For some reason, it translates much fewer words than it should, when given high values of translation probability.

@ceilican
Copy link
Contributor

ceilican commented Mar 1, 2015

Have a look also on the shuffle method. It is responsible for shuffling the list of words extracted from the text, so that we don't end up translating always the same words.

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