Opened 9 years ago

Closed 6 years ago

#11258 closed task (wontfix)

dojox.rails has code in top-level rollup

Reported by: dante Owned by: foobarfighter
Priority: high Milestone: 1.9
Component: Dojox Version: 1.5.0b2
Keywords: Cc:
Blocked By: Blocking:

Description

Typically, the .js files off dojox/ have nothing but require() statements in them. dojox.rails puts a whole rollup with code in dojox/rails.js ... it should be:

dojo.provide("dojox.rails");
dojo.require("dojox.rails._base");

just to preserve convention, and make migrating each to separate projects in the future easier and more consistent.

Also, despite only being used in the tests, plugd/trigger should just be ported/modified to not live in the dojo namespace. The doc parser will find this file and include inline docs for it. Expose dojo.trigger as dojox.rails.test.trigger and create a local alias for the test usage.

Additionally, dojox/rails.js does not follow the Dojo style guidelines consistently. We use tabs set to 4 spaces uniformly across all code. see: http://o.dojotoolkit.org/developer/styleguide for the full rundown. Other than spaces between if and ( and the mixed tab/space indention it seems to be okay though, so minor style issues.

Otherwise, congrats on your first commit!

Change History (6)

comment:1 Changed 9 years ago by foobarfighter

Status: newassigned

Thank you for the ticket! :)

You are absolutely correct about the convention guidelines and I'm planning on fixing all of that asap. I will close this ticket when:

  1. plugd is either removed or moved to the test namespace (is it test or tests?)
  1. The indentation is fixed

The only issue that needs to be raised is the convention for

dojo.provide("dojox.rails");
dojo.require("dojox.rails._base");

The rails people want to see this in a single file (explicitly mentioned by wycats from the rails team). It's much easier to include this code inline then it is to try and create a dojox.rails build when in the dojo repo and since this code will be shared by the rails people in their repo, have a different codebase for the rails people... again, they want one file.

There are a myriad of ways to make this work, but all of them require effort. I am completely happy with complying with the dojox convention if it's truly necessary, but it's much easier to maintain the inline dojox.rails code.

Let me know what you think is best, and I will do it. My goal is to get something that satisfies the dojo peeps as well as the rails peeps and since coordinating two separate projects can be tough, I chose the path of least resistance.

Again, thanks for filing the ticket and I will have the changes in right away.

Regards, foobarfighter/Bob

PS... please keep the code reviews coming.

comment:2 Changed 9 years ago by foobarfighter

@dante

Fixed the style for dojox.rails and plugd.trigger now resides in the dojox.rails.tests.plugd.trigger namespace. Can you take a look please? Also, how strongly do you feel about moving the code from dojox.rails into dojox.rails._base?

comment:3 Changed 9 years ago by Adam Peller

Milestone: 1.51.6

comment:4 Changed 9 years ago by dante

pretty strongly, regarding keep code out of dojox/rails.js

one option, as I know the rails folks want a single file to distro, would be to add a dojox.rails "layer" to standard.profile.js (the one we use for distribution/etc). This would make a "rollup" of dojox.rails._base and all deps in dojox/rails.js after a standard build.

thoughts?

comment:5 Changed 8 years ago by bill

Milestone: 1.6future

(sadly) punting seemingly abandoned ticket and meta tickets to future

comment:6 Changed 6 years ago by dylan

Milestone: future1.9
Resolution: wontfix
Status: assignedclosed

Closing, as this looks abandoned.

Note: See TracTickets for help on using tickets.