Code review comment for lp:~nmarshall23/do-plugins/spellcheck-plugin

Revision history for this message
Chris S. (cszikszoy) wrote :

I haven't had a chance to see if this builds / works, so I'm just going by the merge diff right now.

You seem to have done a very good job integrating this with our build system, thanks!

First and foremost: We require that all new plugin submissions follow Mono's coding style guidelines (here: http://www.mono-project.com/Coding_Guidelines).

It seems unnecessary to create a new instance of Broker every time you run the perform action, why don't you make Broker a property, (private Broker EnchantBroker { get; set; }) and then use the action's constructor to create a new instance of the broker when the action is initialized (ie, when M.A. loads your plugin).

You could greatly simlify the way you return suggested items. For example, using Linq and lamdas you could do:

List<Item> suggestions = new List<Item> ();
dictionary.Suggest (sub).ForEach ( sug => { suggestions.Add (new TextItem (sug) as Item); });
yield return suggestions;

When working with Enumerables, use yield, not plain-old return. So at the end of the perform action, use yield break;

I'm not familiar with Enchant, does it support multiple languages? I see you hardcoded en-US, if it does support multiple languages it would be nice to make a small config widget so that the user could change the language of the dictionary.

Those are a few initial comments, fix those and I'll take another look. When you're done, come back to this merge request, and look near the top. There should be a yellow "edit" icon near the "Status" description. Click that, then on the next page click on "Resubmit". It will ask you to confirm the new revisions to the branch, just click OK. This will make reviewing much easier because it will generate a new and updated merge diff.

Thanks!

review: Needs Fixing

« Back to merge proposal