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

Look through Faraday's Law and its dependencies to make sure correct patterns are being used. #85

Closed
samreid opened this issue Nov 4, 2017 · 12 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Nov 4, 2017

See https://github.com/phetsims/phet-io/issues/1242 and #79

@samreid samreid self-assigned this Nov 4, 2017
samreid added a commit that referenced this issue Nov 4, 2017
samreid added a commit that referenced this issue Nov 4, 2017
samreid added a commit that referenced this issue Nov 4, 2017
samreid added a commit that referenced this issue Nov 4, 2017
@samreid samreid mentioned this issue Nov 4, 2017
91 tasks
@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

As I was checking through this sim to see if phet-io patterns are applied properly, I noticed many parts of the sim that are not up to our code review standards. I created #86 for tracking it. I'll see if I can proceed on this issue before #86 is complete.

samreid added a commit that referenced this issue Nov 4, 2017
@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

I searched through all of the faradays-law files for the string tandem and usages looked great, I saw only 2 places that needed updating (committed above). Next step is to look through dependencies. Not sure how to easily identify all sim dependencies, other than searching for require statements. Maybe doing a requirejs build with --mangle=false would show us what was included?

@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

https://stackoverflow.com/questions/11756483/require-js-access-all-loaded-modules taught me how to use require.s.contexts._.defined to identify all loaded modules. I used

_.keys(require.s.contexts._.defined).sort().join('\n')

and found 330 modules:

../../chipper/js/common/ChipperConstants
../../chipper/js/common/ChipperStringUtils
../../chipper/js/common/getLicenseEntry
../../chipper/js/common/mipmapDownscale
../../chipper/js/data/localeInfo
../../chipper/js/requirejs-plugins/getProjectURL
../../chipper/js/requirejs-plugins/registerLicenseEntry
../../sherpa/lib/lodash-4.17.4.min
AXON/BooleanProperty
AXON/DerivedProperty
AXON/Emitter
AXON/Events
AXON/Multilink
AXON/NumberProperty
AXON/ObservableArray
AXON/Property
AXON/TDerivedProperty
AXON/TEmitter
AXON/TNumberProperty
AXON/TObservableArray
AXON/TProperty
AXON/axon
BRAND/../../js/brand
BRAND/Brand
DOT/BinPacker
DOT/Bounds2
DOT/Dimension2
DOT/Matrix3
DOT/Matrix4
DOT/Random
DOT/Ray2
DOT/TVector2
DOT/Transform3
DOT/Util
DOT/Vector2
DOT/Vector3
DOT/Vector4
DOT/dot
FARADAYS_LAW/faradays-law/FaradaysLawConstants
FARADAYS_LAW/faradays-law/FaradaysLawScreen
FARADAYS_LAW/faradays-law/model/CoilModel
FARADAYS_LAW/faradays-law/model/FaradaysLawModel
FARADAYS_LAW/faradays-law/model/MagnetModel
FARADAYS_LAW/faradays-law/model/VoltmeterModel
FARADAYS_LAW/faradays-law/view/Aligner
FARADAYS_LAW/faradays-law/view/BulbNode
FARADAYS_LAW/faradays-law/view/CoilNode
FARADAYS_LAW/faradays-law/view/CoilTypeEnum
FARADAYS_LAW/faradays-law/view/CoilsWiresNode
FARADAYS_LAW/faradays-law/view/ControlPanelNode
FARADAYS_LAW/faradays-law/view/FaradaysLawScreenView
FARADAYS_LAW/faradays-law/view/MagnetFieldLines
FARADAYS_LAW/faradays-law/view/MagnetNode
FARADAYS_LAW/faradays-law/view/MagnetNodeWithField
FARADAYS_LAW/faradays-law/view/VoltmeterNode
FARADAYS_LAW/faradays-law/view/VoltmeterScale
FARADAYS_LAW/faradays-law/view/VoltmeterWiresNode
FARADAYS_LAW/faradays-law/view/buttons/FlipMagnetButton
FARADAYS_LAW/faradaysLaw
JOIST/AboutDialog
JOIST/CreditsNode
JOIST/Dialog
JOIST/Frame
JOIST/FullScreen
JOIST/HighlightNode
JOIST/HomeButton
JOIST/HomeScreen
JOIST/HomeScreenView
JOIST/JoistA11yStrings
JOIST/JoistButton
JOIST/KeyboardHelpButton
JOIST/KeyboardHelpDialog
JOIST/LinkText
JOIST/LookAndFeel
JOIST/NavigationBar
JOIST/NavigationBarScreenButton
JOIST/OptionsDialog
JOIST/PhetButton
JOIST/PhetMenu
JOIST/Profiler
JOIST/Screen
JOIST/ScreenButton
JOIST/ScreenView
JOIST/ScreenshotGenerator
JOIST/Sim
JOIST/SimLauncher
JOIST/SimVersion
JOIST/TDialog
JOIST/TOptionsDialog
JOIST/TPhetButton
JOIST/TPhetMenu
JOIST/TScreenButton
JOIST/TSim
JOIST/UpdateCheck
JOIST/UpdateDialog
JOIST/UpdateNodes
JOIST/checkNamespaces
JOIST/joist
JOIST/packageJSON
JOIST/thirdPartySupport/LegendsOfLearningSupport
KITE/Shape
KITE/kite
KITE/ops/Boundary
KITE/ops/BoundsIntersection
KITE/ops/Edge
KITE/ops/Face
KITE/ops/Graph
KITE/ops/HalfEdge
KITE/ops/Loop
KITE/ops/Vertex
KITE/parser/svgPath
KITE/segments/Arc
KITE/segments/Cubic
KITE/segments/EllipticalArc
KITE/segments/Line
KITE/segments/Quadratic
KITE/segments/Segment
KITE/util/LineStyles
KITE/util/Overlap
KITE/util/RayIntersection
KITE/util/SegmentIntersection
KITE/util/Subpath
PHETCOMMON/phetcommon
PHETCOMMON/util/StringUtils
PHET_CORE/Namespace
PHET_CORE/Poolable
PHET_CORE/Timer
PHET_CORE/arrayRemove
PHET_CORE/cleanArray
PHET_CORE/detectPrefix
PHET_CORE/detectPrefixEvent
PHET_CORE/escapeHTML
PHET_CORE/extend
PHET_CORE/extendDefined
PHET_CORE/inherit
PHET_CORE/inheritance
PHET_CORE/phetAllocation
PHET_CORE/phetCore
PHET_CORE/platform
PHET_CORE/toCamelCase
SCENERY/accessibility/Accessibility
SCENERY/accessibility/AccessibilityUtil
SCENERY/accessibility/AccessibleInstance
SCENERY/accessibility/AccessiblePeer
SCENERY/accessibility/Focus
SCENERY/accessibility/FocusHighlightFromNode
SCENERY/accessibility/FocusHighlightPath
SCENERY/accessibility/TFocus
SCENERY/display/BackboneDrawable
SCENERY/display/Block
SCENERY/display/CanvasBlock
SCENERY/display/CanvasSelfDrawable
SCENERY/display/ChangeInterval
SCENERY/display/DOMBlock
SCENERY/display/DOMSelfDrawable
SCENERY/display/Display
SCENERY/display/Drawable
SCENERY/display/Fittability
SCENERY/display/FittedBlock
SCENERY/display/GreedyStitcher
SCENERY/display/InlineCanvasCacheDrawable
SCENERY/display/Instance
SCENERY/display/PaintObserver
SCENERY/display/PaintSVGState
SCENERY/display/RebuildStitcher
SCENERY/display/RelativeTransform
SCENERY/display/Renderer
SCENERY/display/SVGBlock
SCENERY/display/SVGGradient
SCENERY/display/SVGGradientStop
SCENERY/display/SVGGroup
SCENERY/display/SVGLinearGradient
SCENERY/display/SVGRadialGradient
SCENERY/display/SVGSelfDrawable
SCENERY/display/SelfDrawable
SCENERY/display/SharedCanvasCacheDrawable
SCENERY/display/Stitcher
SCENERY/display/WebGLBlock
SCENERY/display/WebGLSelfDrawable
SCENERY/display/drawables/CircleCanvasDrawable
SCENERY/display/drawables/CircleDOMDrawable
SCENERY/display/drawables/CircleSVGDrawable
SCENERY/display/drawables/CircleStatefulDrawable
SCENERY/display/drawables/ImageCanvasDrawable
SCENERY/display/drawables/ImageDOMDrawable
SCENERY/display/drawables/ImageSVGDrawable
SCENERY/display/drawables/ImageStatefulDrawable
SCENERY/display/drawables/ImageWebGLDrawable
SCENERY/display/drawables/LineCanvasDrawable
SCENERY/display/drawables/LineSVGDrawable
SCENERY/display/drawables/LineStatefulDrawable
SCENERY/display/drawables/PaintableStatefulDrawable
SCENERY/display/drawables/PaintableStatelessDrawable
SCENERY/display/drawables/PathCanvasDrawable
SCENERY/display/drawables/PathSVGDrawable
SCENERY/display/drawables/PathStatefulDrawable
SCENERY/display/drawables/RectangleCanvasDrawable
SCENERY/display/drawables/RectangleDOMDrawable
SCENERY/display/drawables/RectangleSVGDrawable
SCENERY/display/drawables/RectangleStatefulDrawable
SCENERY/display/drawables/RectangleWebGLDrawable
SCENERY/display/drawables/TextCanvasDrawable
SCENERY/display/drawables/TextDOMDrawable
SCENERY/display/drawables/TextSVGDrawable
SCENERY/display/drawables/TextStatefulDrawable
SCENERY/input/BatchedDOMEvent
SCENERY/input/BrowserEvents
SCENERY/input/ButtonListener
SCENERY/input/DownUpListener
SCENERY/input/Event
SCENERY/input/Input
SCENERY/input/Mouse
SCENERY/input/Pen
SCENERY/input/Pointer
SCENERY/input/SimpleDragHandler
SCENERY/input/TButtonListener
SCENERY/input/TSimpleDragHandler
SCENERY/input/Touch
SCENERY/nodes/AlignBox
SCENERY/nodes/Circle
SCENERY/nodes/HBox
SCENERY/nodes/Image
SCENERY/nodes/LayoutBox
SCENERY/nodes/Leaf
SCENERY/nodes/Line
SCENERY/nodes/Node
SCENERY/nodes/Paintable
SCENERY/nodes/Path
SCENERY/nodes/Plane
SCENERY/nodes/Rectangle
SCENERY/nodes/RichText
SCENERY/nodes/Spacer
SCENERY/nodes/TNode
SCENERY/nodes/TRichText
SCENERY/nodes/TText
SCENERY/nodes/Text
SCENERY/nodes/VBox
SCENERY/nodes/VStrut
SCENERY/overlays/CanvasNodeBoundsOverlay
SCENERY/overlays/FittedBlockBoundsOverlay
SCENERY/overlays/FocusOverlay
SCENERY/overlays/PointerAreaOverlay
SCENERY/overlays/PointerOverlay
SCENERY/overlays/ShapeBasedOverlay
SCENERY/scenery
SCENERY/util/CanvasContextWrapper
SCENERY/util/Color
SCENERY/util/Features
SCENERY/util/Font
SCENERY/util/Gradient
SCENERY/util/LinearGradient
SCENERY/util/Paint
SCENERY/util/Picker
SCENERY/util/RadialGradient
SCENERY/util/RendererSummary
SCENERY/util/SceneryStyle
SCENERY/util/ShaderProgram
SCENERY/util/SpriteSheet
SCENERY/util/TFont
SCENERY/util/TextBounds
SCENERY/util/Trail
SCENERY/util/TrailPointer
SCENERY/util/TransformTracker
SCENERY/util/Util
SCENERY_PHET/ArrowNode
SCENERY_PHET/ArrowShape
SCENERY_PHET/BarrierRectangle
SCENERY_PHET/MinusNode
SCENERY_PHET/MultiLineText
SCENERY_PHET/PhetColorScheme
SCENERY_PHET/PhetFont
SCENERY_PHET/PlusNode
SCENERY_PHET/ResetShape
SCENERY_PHET/SceneryPhetA11yStrings
SCENERY_PHET/ShadedRectangle
SCENERY_PHET/SpinningIndicatorNode
SCENERY_PHET/TBarrierRectangle
SCENERY_PHET/TMultiLineText
SCENERY_PHET/accessibility/AriaHerald
SCENERY_PHET/accessibility/KeyboardDragHandler
SCENERY_PHET/accessibility/TUtteranceQueue
SCENERY_PHET/accessibility/Utterance
SCENERY_PHET/accessibility/UtteranceQueue
SCENERY_PHET/buttons/ResetAllButton
SCENERY_PHET/buttons/ResetButton
SCENERY_PHET/sceneryPhet
SUN/CallbackTimer
SUN/CheckBox
SUN/ColorConstants
SUN/FontAwesomeNode
SUN/MenuItem
SUN/Panel
SUN/TCheckBox
SUN/TMenuItem
SUN/TRadioButtonGroupMember
SUN/buttons/ButtonListener
SUN/buttons/ButtonModel
SUN/buttons/PushButtonInteractionStateProperty
SUN/buttons/PushButtonModel
SUN/buttons/RadioButtonGroup
SUN/buttons/RadioButtonGroupAppearance
SUN/buttons/RadioButtonGroupMember
SUN/buttons/RadioButtonGroupMemberModel
SUN/buttons/RadioButtonInteractionStateProperty
SUN/buttons/RectangularButtonView
SUN/buttons/RectangularPushButton
SUN/buttons/RoundButtonView
SUN/buttons/RoundPushButton
SUN/buttons/TPushButton
SUN/buttons/TPushButtonModel
SUN/buttons/TextPushButton
SUN/sun
TANDEM/Tandem
TANDEM/tandemNamespace
faradays-law-config
faradays-law-main
ifphetio
ifphetio!PHET_IO/SimIFrameAPI
ifphetio!PHET_IO/assertions/assertInstanceOf
ifphetio!PHET_IO/assertions/assertInstanceOfTypes
ifphetio!PHET_IO/phetio
ifphetio!PHET_IO/phetioEvents
ifphetio!PHET_IO/phetioInherit
ifphetio!PHET_IO/types/TBoolean
ifphetio!PHET_IO/types/TFunctionWrapper
ifphetio!PHET_IO/types/TNumber
ifphetio!PHET_IO/types/TObject
ifphetio!PHET_IO/types/TPhETIO
ifphetio!PHET_IO/types/TString
ifphetio!PHET_IO/types/TVoid

@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

This list is overwhelming, we need rules that will help us identify which files need instrumentation and which don't.

UPDATE: maybe we can write a script that will iterate through those 330 files and report which ones include the string tandem or phetio. Since we already have a good approximation of the desired instrumentation, this should be a complete list of files that need attention (may contain false positives but we won't miss anything).

@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

Here's a script that looks through function modules for tandem or phetio, run it after all modules loaded:

    var x = _.keys( window.require.s.contexts._.defined ).sort().filter( function( s ) {
      var content = window.require.s.contexts._.defined[ s ];
      if (typeof content==='function'){
        var string = content.toString();
        if (string.indexOf('tandeem')>=0 || string.indexOf('phetio')>=0){
          return true;
        }
      }
    } ).join( '\n' );
    console.log(x);

For Faraday's Law, I get:

AXON/BooleanProperty
AXON/DerivedProperty
AXON/Emitter
AXON/NumberProperty
AXON/ObservableArray
AXON/Property
AXON/TDerivedProperty
AXON/TEmitter
AXON/TNumberProperty
AXON/TObservableArray
AXON/TProperty
DOT/TVector2
FARADAYS_LAW/faradays-law/model/FaradaysLawModel
FARADAYS_LAW/faradays-law/model/MagnetModel
JOIST/Dialog
JOIST/OptionsDialog
JOIST/PhetMenu
JOIST/Screen
JOIST/ScreenButton
JOIST/Sim
JOIST/TDialog
JOIST/TOptionsDialog
JOIST/TPhetButton
JOIST/TPhetMenu
JOIST/TScreenButton
JOIST/TSim
SCENERY/accessibility/TFocus
SCENERY/input/SimpleDragHandler
SCENERY/input/TButtonListener
SCENERY/input/TSimpleDragHandler
SCENERY/nodes/Node
SCENERY/nodes/RichText
SCENERY/nodes/TNode
SCENERY/nodes/TRichText
SCENERY/nodes/TText
SCENERY/nodes/Text
SCENERY/util/TFont
SCENERY_PHET/BarrierRectangle
SCENERY_PHET/MultiLineText
SCENERY_PHET/TBarrierRectangle
SCENERY_PHET/TMultiLineText
SCENERY_PHET/accessibility/TUtteranceQueue
SCENERY_PHET/buttons/ResetAllButton
SUN/CheckBox
SUN/MenuItem
SUN/TCheckBox
SUN/TMenuItem
SUN/TRadioButtonGroupMember
SUN/buttons/PushButtonModel
SUN/buttons/RadioButtonGroup
SUN/buttons/RadioButtonGroupMember
SUN/buttons/RadioButtonGroupMemberModel
SUN/buttons/RectangularPushButton
SUN/buttons/RoundPushButton
SUN/buttons/TPushButton
SUN/buttons/TPushButtonModel

This doesn't seem right because text search reveals 12 files in faraday's law with the term tandem.

@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

Oops, tandeem is a typo... please hold....

    var x = _.keys( window.require.s.contexts._.defined ).sort().filter( function( s ) {
      var content = window.require.s.contexts._.defined[ s ];
      if (typeof content==='function'){
        var string = content.toString();
        if (string.indexOf('tandem')>=0 || string.indexOf('phetio')>=0){
          return true;
        }
      }
    } ).map(function(r){return ' - [ ] '+r}).join( '\n' );
    console.log(x);

@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

  • AXON/BooleanProperty
  • AXON/DerivedProperty
  • AXON/Emitter
  • AXON/NumberProperty
  • AXON/ObservableArray
  • AXON/Property
  • AXON/TDerivedProperty
  • AXON/TEmitter
  • AXON/TNumberProperty
  • AXON/TObservableArray
  • AXON/TProperty
  • DOT/TVector2
  • FARADAYS_LAW/faradays-law/FaradaysLawScreen
  • FARADAYS_LAW/faradays-law/model/FaradaysLawModel
  • FARADAYS_LAW/faradays-law/model/MagnetModel
  • FARADAYS_LAW/faradays-law/model/VoltmeterModel
  • FARADAYS_LAW/faradays-law/view/ControlPanelNode
  • FARADAYS_LAW/faradays-law/view/FaradaysLawScreenView
  • FARADAYS_LAW/faradays-law/view/MagnetNodeWithField
  • FARADAYS_LAW/faradays-law/view/VoltmeterNode
  • FARADAYS_LAW/faradays-law/view/buttons/FlipMagnetButton
  • JOIST/Dialog
  • JOIST/HomeButton
  • JOIST/HomeScreen
  • JOIST/HomeScreenView
  • JOIST/JoistButton
  • JOIST/KeyboardHelpButton
  • JOIST/KeyboardHelpDialog
  • JOIST/NavigationBar
  • JOIST/NavigationBarScreenButton
  • JOIST/OptionsDialog
  • JOIST/PhetButton
  • JOIST/PhetMenu
  • JOIST/Screen
  • JOIST/ScreenButton
  • JOIST/ScreenView
  • JOIST/Sim
  • JOIST/TDialog
  • JOIST/TOptionsDialog
  • JOIST/TPhetButton
  • JOIST/TPhetMenu
  • JOIST/TScreenButton
  • JOIST/TSim
  • JOIST/checkNamespaces
  • SCENERY/accessibility/TFocus
  • SCENERY/input/ButtonListener
  • SCENERY/input/SimpleDragHandler
  • SCENERY/input/TButtonListener
  • SCENERY/input/TSimpleDragHandler
  • SCENERY/nodes/Node
  • SCENERY/nodes/RichText
  • SCENERY/nodes/TNode
  • SCENERY/nodes/TRichText
  • SCENERY/nodes/TText
  • SCENERY/nodes/Text
  • SCENERY/util/TFont
  • SCENERY_PHET/ArrowNode
  • SCENERY_PHET/BarrierRectangle
  • SCENERY_PHET/MultiLineText
  • SCENERY_PHET/PhetFont
  • SCENERY_PHET/TBarrierRectangle
  • SCENERY_PHET/TMultiLineText
  • SCENERY_PHET/accessibility/TUtteranceQueue
  • SCENERY_PHET/buttons/ResetAllButton
  • SCENERY_PHET/buttons/ResetButton
  • SUN/CheckBox
  • SUN/MenuItem
  • SUN/TCheckBox
  • SUN/TMenuItem
  • SUN/TRadioButtonGroupMember
  • SUN/buttons/PushButtonModel
  • SUN/buttons/RadioButtonGroup
  • SUN/buttons/RadioButtonGroupMember
  • SUN/buttons/RadioButtonGroupMemberModel
  • SUN/buttons/RectangularPushButton
  • SUN/buttons/RoundPushButton
  • SUN/buttons/TPushButton
  • SUN/buttons/TPushButtonModel
  • SUN/buttons/TextPushButton
  • TANDEM/Tandem

samreid added a commit to phetsims/axon that referenced this issue Nov 4, 2017
@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

@zepumph can you please review the preceding commit?

@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

@zepumph can you please review the preceding commit as well? I think I'm at a good point to check in with @zepumph and make sure I'm barking up the right tree before going too much further. Unassigning until then.

@samreid samreid removed their assignment Nov 4, 2017
@zepumph
Copy link
Member

zepumph commented Nov 6, 2017

I'm not sure if adding the isLegalAndUsable calls in phetsims/axon@aeb1bbf is the latest and greatest practice. That should be decided in https://github.com/phetsims/phet-io/issues/1235

As for phetsims/axon@e9f22ab: looks good.

@zepumph
Copy link
Member

zepumph commented Jan 3, 2018

Back to you @samreid, sorry it took so long.

@zepumph zepumph assigned samreid and unassigned zepumph Jan 3, 2018
samreid added a commit to phetsims/axon that referenced this issue Jan 21, 2018
samreid added a commit to phetsims/axon that referenced this issue Jan 21, 2018
samreid added a commit to phetsims/axon that referenced this issue Jan 21, 2018
samreid added a commit to phetsims/dot that referenced this issue Jan 21, 2018
samreid added a commit to phetsims/sun that referenced this issue Jan 21, 2018
@samreid
Copy link
Member Author

samreid commented Feb 28, 2018

Now that we've standardized on PhetioObject and use startEvent/endEvent for messaging, it is more likely that instrumented objects are properly instrumented. Furthermore, Faraday's Law only exposes 20 types over the PhET-iO API:

PhETIO
PhetioCommandProcessorIO
PropertyIO.<BooleanIO>
SimIO
NumberPropertyIO
TextIO
MenuItemIO
PhetMenuIO
JoistButtonIO
PropertyIO.<Vector2IO>
PropertyIO.<StringIO>
NodeIO
PushButtonIO
DerivedPropertyIO.<BooleanIO>
RadioButtonGroupMemberIO
CheckboxIO
SimpleDragHandlerIO
BarrierRectangleIO

Many of these types such as PhETIO or NumberProperty have been refined and do not need another pass at the moment. Many Node subtypes such as Node and Checkbox get their behavior from PhetioObject and hence only need to verify that the correct phet-io options are passed to PhetioObject, and we can tell most of those are OK from Instance Proxies.

As an example spot check, Checkbox.js only needs to supply tandem and phetioType, and it is doing so nicely.

I'd like to close this issue.

@samreid samreid closed this as completed Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants