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

Easy post-load checking of namespacing #307

Closed
jonathanolson opened this issue Nov 16, 2015 · 40 comments
Closed

Easy post-load checking of namespacing #307

jonathanolson opened this issue Nov 16, 2015 · 40 comments

Comments

@jonathanolson
Copy link
Contributor

As an alternative to phetsims/tasks#435 (require.js-based) or phetsims/tasks#431 (eslint-based) for checking the namespacing of phetsims/tasks#378.

We had already created a debugging tool for creating these, and I've repurposed it so that if you run in require.js mode with ?ea&checkNamespaces, it will check require.js modules AFTER they all have been loaded, verifying that they exist at their expected namespace locations.

@jonathanolson jonathanolson self-assigned this Nov 16, 2015
jonathanolson added a commit to phetsims/chipper that referenced this issue Nov 16, 2015
jonathanolson added a commit that referenced this issue Nov 16, 2015
@jonathanolson
Copy link
Contributor Author

Added to discuss using this as our sole namespace checking method (and enabling by default once we have all common code namespaced). This would mean bailing on phetsims/tasks#435 (require.js-based namespace checks) and phetsims/tasks#431 (eslint-based namespace checks).

@pixelzoom
Copy link
Contributor

How do I test drive this? I tried:

http://localhost/~cmalley/Github/hookes-law/hookes-law_en.html?ea&checkNamespaces

... and I get this assertion failure:

screenshot_94

Is this because joist isn't namespaced yet?

@samreid
Copy link
Member

samreid commented Nov 17, 2015

Yes, it asserts that all of the requirejs modules appear in the global namespace with the appropriate name and value. It is failing on the module "checkNamespaces" which has not been namespaced.

@samreid
Copy link
Member

samreid commented Nov 17, 2015

You can negate the assert conditions and transform them to if() console.log() if you want to see a full list of offenders.

@samreid
Copy link
Member

samreid commented Nov 17, 2015

I asked @jonathanolson over skype:

Brainstorming a change (improvement?) for checkNamespaces.js. How about it first prints out all offending namespace issues in one pass before assert()ing anything?

@samreid
Copy link
Member

samreid commented Nov 17, 2015

@jonathanolson responded:

That change sounds fine to me

@samreid
Copy link
Member

samreid commented Nov 17, 2015

How about it first prints out all offending namespace issues in one pass before assert()ing anything?

I have done this on my working copy but want to test it further before committing. Tomorrow.

@samreid
Copy link
Member

samreid commented Nov 17, 2015

Fixed above. Leaving open and marked for developer-meeting for discussion of how to use this and its limitations (only checks AMD modules loaded without a plugin).

@pixelzoom
Copy link
Contributor

Very odd that checkNamespace by itself does nothing. Why does this features require 2 query parameters? (?ea&checkNamespaces)

How about checkNamespace to get the console output without assertion failure, and add ea to enable related assertions?

@samreid
Copy link
Member

samreid commented Nov 17, 2015

Consensus: We will keep the checkNamespace option for now to help developers namespace sims, in the future we will remove ?checkNamespaces and just use ?ea to throw assertion errors if a namespacing issue occurs.

@pixelzoom
Copy link
Contributor

Limitations:
• won't confirm namespacing of inner types
• there are some non-require things that need to be namespaced in scenery

We can live with these.

@pixelzoom
Copy link
Contributor

In Skype, I proposed that it's time to enable namespace checking. Discussion:

[5/2/16, 12:54:37 PM] Chris Malley: Re turning on checkNamespace... I see lots of recent code (expression-exchange-basics, CCK, making-tens,...) that is not namespaced. Looks like developers have been generally blowing this off. Is that what we decided to do?
[5/2/16, 12:56:03 PM] Chris Malley: I understand for old code, but I'm a bit disappointed for recent code.
[5/2/16, 12:56:48 PM] Sam Reid: For new code I often work in phases where I tackle cross cutting concerns such as i18n and namespacing as separate step. CCK isn’t i18n’ed yet either. Flipping the switch to require namespaces will encourage me to get to that step much sooner! (and I’m OK with that)
[5/2/16, 12:59:23 PM] Chris Malley: How about new sims, like Neuron? 1.0.0 published in Feb, a full 3 months after we decided to namespace. There's 1 source file that's properly namespaced.
[5/2/16, 12:59:39 PM] John Blanco: Me too - it'll force me to get these things done (or delegate some of them to Aadish).

@pixelzoom
Copy link
Contributor

Assigned to @jonathanolson to review. Then I believe this can be closed.

@pixelzoom
Copy link
Contributor

@phet-steele FYI, here's the current state of affairs when running test-server. The majority of runnable (sims and demos) are failing with "Assertion failed: not namespaced".

screenshot_20
screenshot_21

@pixelzoom
Copy link
Contributor

pixelzoom commented May 2, 2016

I namespaced blast, chains, griddle, graphing-quadratics, ohm-law, resistance-in-a-wire.

@jonathanolson
Copy link
Contributor Author

I'll plan to tackle build-a-molecule, making-tens, pendulum-lab, wave-on-a-string.

@jbphet
Copy link
Contributor

jbphet commented May 2, 2016

Neuron, Area Builder, and Arithmetic have been namespaced. The 3 sims that comprise the States of Matter suite have been assigned to @aadish, see phetsims/states-of-matter#114. I'll tackle Balancing Act and Expression Exchange tomorrow.

@aadish
Copy link

aadish commented May 3, 2016

namespaced build an atom in above commit

jessegreenberg added a commit to phetsims/john-travoltage that referenced this issue May 3, 2016
jonathanolson added a commit to phetsims/wave-on-a-string that referenced this issue May 3, 2016
@jbphet
Copy link
Contributor

jbphet commented May 3, 2016

Balancing Act and Expression Exchange are now namespaced.

@aadish
Copy link

aadish commented May 3, 2016

States of Matter, Atomic Interactions and States of Matter Basics are namespaced

jessegreenberg added a commit to phetsims/gravity-and-orbits that referenced this issue May 3, 2016
@jessegreenberg
Copy link
Contributor

john-travoltage and gravity-and-orbits have been namespaced.

@phet-steele
Copy link

phet-steele commented May 10, 2016

Hey all, still left:

@aadish
Copy link

aadish commented May 10, 2016

my bad pushed the commit for build and atom on wrong branch

@jbphet
Copy link
Contributor

jbphet commented May 11, 2016

Should now be fixed for Balancing Act. I had done the same thing as @aadish, i.e. I committed the changes on a branch instead of on master.

@jbphet
Copy link
Contributor

jbphet commented May 11, 2016

Marking for developer meeting so that we can discuss and assign individuals to finish the remaining sims.

jbphet added a commit to phetsims/projectile-motion that referenced this issue May 11, 2016
@jbphet
Copy link
Contributor

jbphet commented May 11, 2016

I went ahead and did projectile-motion, since I set it up initially and there isn't much to it yet.

pixelzoom added a commit to phetsims/calculus-grapher that referenced this issue May 12, 2016
pixelzoom added a commit to phetsims/curve-fitting that referenced this issue May 12, 2016
pixelzoom added a commit to phetsims/fraction-matcher that referenced this issue May 12, 2016
pixelzoom added a commit to phetsims/fraction-comparison that referenced this issue May 12, 2016
pixelzoom added a commit to phetsims/pendulum-lab that referenced this issue May 12, 2016
pixelzoom added a commit to phetsims/make-a-ten that referenced this issue May 12, 2016
pixelzoom added a commit to phetsims/plinko-probability that referenced this issue May 12, 2016
@pixelzoom
Copy link
Contributor

I took care of all remaining sims, with one exception. build-a-molecule has its own non-standard namespace, and will need to be converted by @jonathanolson (tracking in phetsims/build-a-molecule#78).

@pixelzoom
Copy link
Contributor

pixelzoom commented May 12, 2016

@phet-steele Please kick off an automated test. Verify that the only runnable that fails with "Assertion failed: not namespaced" is build-a-molecule. If verified, close this issue. Otherwise note runnable that still need attention. Thanks.

@phet-steele
Copy link

build-a-molecule is all that's left! Congrats! 🎉 🍰 👍 😄

@pixelzoom
Copy link
Contributor

Excellent, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants