Hello,

Lingwo is a small family of modules for creating an online dictionary, which try to support modern, professional lexicographical practices as much as possible. It focuses on translating dictionaries (ex. Spanish-English dictionary) which are maintained collaboratively by a team, like a wiki.

Here is my sandbox project:

http://drupal.org/sandbox/dsnopek/1073982

I've made several good faith attempts to see if my code could be merged into the "dictionary" module already on Drupal.org to avoid duplication. However, it appears that our goals are simply too different.

I'm aiming to create a more featureful dictionary that takes as much as possible from professional lexicography (dictionary writing). While the "dictionary" project aims to be a simple glossary of terms. We also vary drastically from an implementation perspective: "lingwo" uses nodes for dictionary entries, "dictionary" uses custom tables. "lingwo" uses the standard translation system from "translation" to make entries multi-lingual, "dictionary" uses custom admin pages.

However, since the use cases are so different, I don't feel like this duplication is necessarily bad! Users looking for a simple glossary of terms can get that from "dictionary". Users looking to run a professional-quality, collaborative dictionary building project can come to "lingwo".

Thanks in advance for your time spent reviewing my application!

Best regards,
David Snopek.

Comments

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Needs review
sreynen’s picture

sreynen commented
Status: Needs review » Needs work

Hi dsnopek,

This sounds like an interesting set of modules. I just started reviewing and opened an issue, [#1127914]. Hopefully it won't be a big problem to remove the non-GPL files. Please set this back to "needs review" when that's resolved and I'll look at the updated code.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review

Hi sreynen,

That issue has been addressed: all the non-GPL code has been replaced. Thanks for reviewing!

Best regards,
David.

lslinnet’s picture

lslinnet commented
Status: Needs review » Needs work

After enabling only the lingwo_entry module i get a fatal error when visiting
http://d6/node/add/lingwo-entry
Fatal error: Call to a member function getValue() on a non-object in /Users/lslinnet/Sites/drupal6/sites/all/modules/lingwo/includes/language.inc on line 49
stack trace of the error:
http://cl.ly/0F360u3J1U2a3G184730

The module list looks like this:
Package Name Type Status Version
Administration Admin (admin) Module Enabled 6.x-2.0
Core - optional Aggregator (aggregator) Module Not installed 6.20
Core - optional Blog (blog) Module Not installed 6.20
Core - optional Blog API (blogapi) Module Not installed 6.20
Core - optional Book (book) Module Not installed 6.20
Core - optional Color (color) Module Enabled 6.20
Core - optional Comment (comment) Module Enabled 6.20
Core - optional Contact (contact) Module Not installed 6.20
Core - optional Content translation (translation) Module Not installed 6.20
Core - optional Database logging (dblog) Module Enabled 6.20
Core - optional Forum (forum) Module Not installed 6.20
Core - optional Help (help) Module Enabled 6.20
Core - optional Locale (locale) Module Not installed 6.20
Core - optional Menu (menu) Module Enabled 6.20
Core - optional OpenID (openid) Module Not installed 6.20
Core - optional Path (path) Module Not installed 6.20
Core - optional PHP filter (php) Module Not installed 6.20
Core - optional Ping (ping) Module Not installed 6.20
Core - optional Poll (poll) Module Not installed 6.20
Core - optional Profile (profile) Module Not installed 6.20
Core - optional Search (search) Module Not installed 6.20
Core - optional Statistics (statistics) Module Not installed 6.20
Core - optional Syslog (syslog) Module Not installed 6.20
Core - optional Taxonomy (taxonomy) Module Enabled 6.20
Core - optional Throttle (throttle) Module Not installed 6.20
Core - optional Tracker (tracker) Module Not installed 6.20
Core - optional Trigger (trigger) Module Not installed 6.20
Core - optional Update status (update) Module Enabled 6.20
Core - optional Upload (upload) Module Not installed 6.20
Core - required Block (block) Module Enabled 6.20
Core - required Filter (filter) Module Enabled 6.20
Core - required Node (node) Module Enabled 6.20
Core - required System (system) Module Enabled 6.20
Core - required User (user) Module Enabled 6.20
Lingwo dictionary Lingwo entry (lingwo_entry) Module Enabled
Lingwo dictionary Lingwo fields (lingwo_fields) Module Not installed
Lingwo dictionary Lingwo language (lingwo_language) Module Not installed
Lingwo dictionary Lingwo senses (lingwo_senses) Module Not installed
Other Module filter (module_filter) Module Enabled 6.x-1.6
Other Bluemarine (bluemarine) Theme Disabled 6.20
Other Chameleon (chameleon) Theme Disabled 6.20
Other Garland (garland) Theme Enabled 6.20
Other Marvin (marvin) Theme Disabled 6.20
Other Minnelli (minnelli) Theme Disabled 6.20
Other Pushbutton (pushbutton) Theme Disabled 6.20

lslinnet’s picture

lslinnet commented

Also did a little code style review - this is not a complete review of all the modules put only of the files mentioned:

$Id$ should be removed, Drupal has moved away from CVS and is no longer using these.

includes/ahah.inc
Line 10: missing a space before the first . on the line.

includes/misc.inc
Line 40: missing spaces between concatenated strings (http://drupal.org/coding-standards#concat)

includes/settings.inc
Line 40, 42: concatenated strings should have a space before and after the .

js/util/declare.js
Indentation should be 2 spaces not 4

js/util/extendPrototype.js
Indentation should be 2 spaces not 4

js/Entry.js
Indentation should be 2 spaces not 4
Line 91: bracket could be moved up after ") "
Line 95: Whitespace at EOL
Line 118, 254, 256: i would follow the same standard as in PHP with a space before and after the operator (e.g. " + ")

js/Language.js
Indentation should be 2 spaces not 4
Line 87, 100: i would follow the same standard as in PHP with a space before and after the operator (e.g. " + ")
Line 303: consider commenting out the debugging information.

js/require-stubs.js
Indentation should be 2 spaces not 4
Line 14: i would follow the same standard as in PHP with a space before and after the operator (e.g. " + ")

lingwo_entry/lingwo_entry.api.inc
Line 97, 119, 143, 156: missing spaces around operators (e.g. " = ")
Line 110: missing space between concatenation .

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Hi lslinnet,

Ack! I fixed the "Fatal error". That's quite embarrassing! It was a bug I introduced at some point that only happened when lingwo_language is disabled and, unfortunately, I test without lingwo_language very rarely. Sorry, about that! In the future, I will test with all combinations of modules before making a release. Bugs like this may still sneak into git master, though.

I fixed all of the coding style errors that you point out AND performed a massive audit of my code, looking for the following problems:

  • function argument defaults with missing space around equals operator
  • old style, Drupal 6 string concatenation (with dot operator directly against a quote)
  • JavaScript without spaces around the "+" operator
  • JavaScript files with tabs of 4 spaces instead of 2
  • whitespace at EOL

With the help of coder and grep, I fixed all the problems I could find! Please let me know if you notice any other coding standards issues.

For the record: to the best of my knowledge the new string concatenation standards do not apply to Drupal 6 modules, per this ticket and the ensuing discussion by the coder module developers. But I've decided to upgrade to the new standard anyway, in preparation for porting my modules to Drupal 7.

Thanks so much for taking the time to review my module!

Best regards,
David.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Needs work

I added a critical security issue - [#1145470].

I reviewed some of the rest of the code, but not all.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review

dsnopek pointed out that this is in fact the proper way to rebuild a form and not submit it according to the docs.

Back to "needs review."

ralt’s picture

ralt commented
Component: new project application » module
Priority: Normal » Critical

Changing priority according to the new priority guidelines.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Needs review » Reviewed & tested by the community

This is quite a big set of modules :)
I have been going over a large part of the files, and it all seems very well written and according to Drupal best practices & coding standards.

One minor remark:
You might want to add some extra documentation for some functions, e.g. in lingwo_XXX.api.inc files, so it's clearer what's going on in there.

Seems good to be promoted to full project as far as I'm concerned.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Thanks so much for the vote of confidence! You are right, I should document the lingwo_XXX.api.inc files more. I've added a note about it to my TODO and will do so as soon as I can.

Best regards,
David.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Fixed

You have been granted the ability to create full projects. Use this power sparingly and only when appropriate! Congratulations.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Ah, crap! I already screwed this up. :-/

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.