630387 - Tracking Bug: Shared Modules Refactor -- Sprint 1

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Project tracking bug for Sprint 1 of the Shared Modules refactor, week ending Feb 4th, 2011. Individual tasks will be added as dependencies.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Geo, do you plan to remove remaining items over to sprint 2?

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

It's probably the right thing to do, but I planned on finishing them as sprint 1 and taking care of their review as part of the sprint 1 code review as a matter of convenience. But you're right, that's not really playing the game as I'd set it out.

Let's talk more tomorrow and figure out if we should move the remaining items and review them at the end of sprint 2 instead.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Attachment: Branch diff for sprint 1

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

As long as we haven't reviewed the code and merged to master it doesn't make sense to resolve this bug. Reopening for the time being.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Also please tell me the exact command to run for jsdoc toolkit to generate the docs, so I can verify the output. Thanks.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Henrik,

java -jar jsrun.jar app/run.js -r -t=templates/jsdoc ~/git/mozmill-api-refactor/modules/

There'll be garbage in globals namespace because of the random pseudo-jsdoc in other files. You can specifically run it against ui/widgets.js to see my specific changes. 

Also, sure, re: Resolved, you're right. Thanks for the reopen.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Comment on attachment 514278 [details] [diff] [review]
Branch diff for sprint 1

>diff --git a/modules/ui/widgets.js b/modules/ui/widgets.js
>index cfa7ec3..d7a3491 100644
>--- a/modules/ui/widgets.js
>+++ b/modules/ui/widgets.js
>@@ -30,26 +30,43 @@
Just some comment and nits.

>+ * @namespace Defines proxy classes for creation of a hierarchal map

Should be 'hierarchical'

>+var Element = Inheritance.Class.extend(

Why do we extend the class here? Shouldn't we use Class.create for the base class?

>+/** @lends widgets.Element */

Question: Does it mean that the whole following block will be treated as 'lends'?

>+   * @class A proxy for an element of a DOM
>+   * @constructs
>+   * @memberOf widgets
>+   */
>   initialize: function Element_initialize(locatorType, locator, owner) {

What about the documentation for parameters? It's mostly across the file. Which way do we want to use.

>   get document() {
>   get controller() {
>   get window() {
>   get elem() {
>   get node() {

All the properties and methods have to also be documented. Given the sprint 1 description and dependent bug the elements should have been documented completely. I miss a couple of those documentation across the targeted classes.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #7)

> >+ * @namespace Defines proxy classes for creation of a hierarchal map
> 
> Should be 'hierarchical'

Hah, good catch. I'll change.

> 
> >+var Element = Inheritance.Class.extend(
> 
> Why do we extend the class here? Shouldn't we use Class.create for the base
> class?

Also good point. That's been there since my proof-of-concept, I'll change.

> 
> >+/** @lends widgets.Element */
> 
> Question: Does it mean that the whole following block will be treated as
> 'lends'?
> 
> >+   * @class A proxy for an element of a DOM
> >+   * @constructs
> >+   * @memberOf widgets
> >+   */
> >   initialize: function Element_initialize(locatorType, locator, owner) {

No, not exactly. @lends means that the anonymous json object passed into extend/create (i.e. the one that's the whole class definition) should be treated as the class. See here:

http://code.google.com/p/jsdoc-toolkit/wiki/TagLends

It's also responsible for my having to change the formatting and put the { of the anon object on a new line. It wasn't recognized correctly when I did it inline.

> 
> What about the documentation for parameters? It's mostly across the file. Which
> way do we want to use.

The parameters are part of Element.initialize(), and only echoed to the other initialize() constructors because of the jsdoc constructor inheritance issue I mentioned to you earlier. Once I document Element.initialize, that documentation will be copied to other constructors.

Element is documented in Sprint 2. Please consider this a review the stuff with +/- next to it (i.e. the sprint changes). I'm fine if you want to go through github and post concerns on existing code from the proof of concept too, but best to do that against master branch.

> 
> >   get document() {
> >   get controller() {
> >   get window() {
> >   get elem() {
> >   get node() {
> 
> All the properties and methods have to also be documented. Given the sprint 1
> description and dependent bug the elements should have been documented
> completely. I miss a couple of those documentation across the targeted classes.

Element and HtmlXulElement docs were moved to Sprint 2. You were the one who suggested I move them, so you've simply forgotten this. :) 

All classes will be doc'd before we're done. I spent Sprint 1 mostly figuring out the documentation pattern and applying it to the "easy" classes.

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Henrik, 

Typos are fixed in sprint1 branch with this commit:

https://github.com/geoelectric/mozmill-api-refactor/commit/1dd8298405f8598a454b4c18a33911ea4f76c7ff

I don't have time to generate the diff for you right now. If you could just get started looking at that, I'd appreciate it. 

As I've stated before, we're not doing formal code reviews yet while we're in incubation--our bugs are only there for tracking purposes, not to specifically hang open on attachments and r+, so if you can simply approve the commit so I can merge I can close out sprint 1 and fix the rest of the sprint 2 branches.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to comment #9)
> Typos are fixed in sprint1 branch with this commit:
> https://github.com/geoelectric/mozmill-api-refactor/commit/1dd8298405f8598a454b4c18a33911ea4f76c7ff

Tiny change which looks fine.

> As I've stated before, we're not doing formal code reviews yet while we're in
> incubation--our bugs are only there for tracking purposes, not to specifically

Then you should not request review but feedback. Both have a completely different meaning. For reviews you will get all sorts of comments as someone can expect from it. And comments beside a request will help to understand what has been left-out.

> hang open on attachments and r+, so if you can simply approve the commit so I
> can merge I can close out sprint 1 and fix the rest of the sprint 2 branches.

So after a bit more of thinking that causes a problem for me. If we already merge everything to master, there will be no way for us to finally create a diff of specific code changes. So how would we be able to do final code reviews? We should develop a better plan.

(In reply to comment #8)
> All classes will be doc'd before we're done. I spent Sprint 1 mostly figuring
> out the documentation pattern and applying it to the "easy" classes.

Then it should also be better documented on bugs. None of the dependent ones reflect that. Instead "Document the HtmlRegion class found in xyz" has been used. Next time simply add a comment and explain your patch and what specifics have been left out. I can't remember everything discussed beside bugs.

Comment 11

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #10)

> Then you should not request review but feedback. Both have a completely
> different meaning. For reviews you will get all sorts of comments as someone
> can expect from it. And comments beside a request will help to understand what
> has been left-out.

Point taken. And you're right, I should've given you a better heads up on some of the implementation details there.

> So after a bit more of thinking that causes a problem for me. If we already
> merge everything to master, there will be no way for us to finally create a
> diff of specific code changes. So how would we be able to do final code
> reviews? We should develop a better plan.

We talked a little on IRC, and we're going to talk more Monday about figuring out a good tracking strategy.

In short, though, I do want reviews on a per-sprint basis (or per-feature, if that's how we're going to structure our private branches). So the diff is "code after the last sprint" (i.e. master) vs. "my branch with new changes" (i.e. feature/sprint branch). This will require a cross-merge after I push a last-sprint branch up to master after review; you'll need to pull it and merge master into your outstanding new-sprint branches.

So, there will be a clean diff. I just want to figure out how to speed the reviews up while we're incubating. I do understand that means we're incurring technical debt, and that the code will need more scrutiny later.

That's where milestones come in; we've committed to a short refactoring round once we've reached a milestone. That'll be our opportunity to go over the code again and not only call out any defects we see, but also rearrange it a little to make it cleaner and pay back any technical debt we've incurred by rushing through sprints. I'll snapshot at milestones so we can get diffs between them, too.

The idea is to not get hung up on small changes and worries on a day over day basis, but still have a process where we can polish. I know this is a little different than our process for working on already-existing code, but I think it or something like it is necessary to collaborate well.

But as said before, we'll talk.

> Then it should also be better documented on bugs. None of the dependent ones
> reflect that. Instead "Document the HtmlRegion class found in xyz" has been
> used. Next time simply add a comment and explain your patch and what specifics
> have been left out. I can't remember everything discussed beside bugs.

Best bets are A) check the project wiki, which I can annotate better to represent an entire sprint, and B) show the dependency tree (including resolved bugs) so you can see a clear list of what's included in the sprint.

And again, if this proves to not include important information you need, we'll find a way to make it clearer. This is a process still under construction, so your input is awesome.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

All of that sounds fine! Lets get this landed so we can try out the already established method over the next couple of days. Merging back code changes to individual feature branches isn't such a hard deal. Git is a way better in resolving merge conflicts. But as you say, lets find the best way for us.

Once you have gotten this project branch merged with the master let me know. If you could squash all the existing commits on the branch first before merging it with master it would be perfect. Something identical what we are doing with Mozmill:

https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Ready_to_Push

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Henrik, somehow everything but the last two fixes was already merged to master. I'm unsure if I hosed a git command or what. So, I can only squash the typo/trailing space commits. Apologies about that.

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Oh, I see what happened. I did part of the sprint1 work on master before we decided to use sprint branches. I can squash the commits on master and repush.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

As given via email this branch has been merged now. So can we close this bug?

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Yep, closed.