Rename options to config in annotator #404

seanh · 2017-05-25T19:32:59Z

Another pre-step that I wanted to get in before I propose a refactoring of the settings/config/options code:

The config() function that creates the config (formerly options)
object is renamed to configure() so that the name doesn't clash with
that of the config object. configure() is also the name used for the
equivalent function in h.

There are two reasons for renaming the options object to config:

First, it's more consistent. The file that creates this object is
called config.js, and the function that creates the object is called
configure(), and yet the object ends up getting called options (but
you have to look in another file, main.js, to find this out).

config is also used elsewhere as the name for the main configuration
object, for example in Pyramid/h, in the client docs ("Configuring the
Client", "configuration settings") and public API
(class="js-hypothesis-config" scripts and window.hypothesisConfig()
functions), etc.

These "options" that the src/annotator/ code reads from the host page
also end up getting renamed to hostPageConfig when they get passed
over in to the src/sidebar/ code.

Second, it's more unique. There are a number of other objects in the
src/annotator/ code that are called options, sometimes the main
options object and another local options object are even used in the
same function. There's no other objects called config.

seanh · 2017-05-25T19:34:24Z

This was pretty confusing to do as there are lots of objects named options in the src/annotator/ code that aren't the options object, and in some cases the options object is used in the same function as another different object named options. In some cases config settings that come from the options object are copied into a local options object and then accessed from there, but it's not apparent where the copying actually happens. I think I got every use of the options object and no false-positives, but I'd appreciate a close look at this.

codecov-io · 2017-05-25T19:36:45Z

Codecov Report

Merging #404 into master will not change coverage.
The diff coverage is 95.83%.

@@           Coverage Diff           @@
##           master     #404   +/-   ##
=======================================
  Coverage   89.65%   89.65%           
=======================================
  Files         123      123           
  Lines        4910     4910           
  Branches      846      846           
=======================================
  Hits         4402     4402           
  Misses        508      508

Impacted Files	Coverage Δ
src/annotator/plugin/bucket-bar.coffee	`28.92% <ø> (ø)`	⬆️
src/annotator/config.js	`100% <100%> (ø)`	⬆️
src/annotator/delegator.coffee	`100% <100%> (ø)`	⬆️
src/annotator/guest.coffee	`79.09% <100%> (ø)`	⬆️
src/annotator/sidebar.coffee	`81.63% <100%> (ø)`	⬆️
src/annotator/host.coffee	`96.87% <88.88%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c990769...4ecd8cc. Read the comment docs.

seanh · 2017-05-26T10:28:31Z

If you grep for "config" now then, at least in the annotator code, all the files and objects named config refer to the main config object and nothing else:

$ ag -g config src/annotator/
src/annotator/config.js
src/annotator/test/config-test.js

$ src/annotator/config.js:9: * Reads the Hypothesis configuration from the environment.
src/annotator/config.js:11: * @param {Window} window_ - The Window object to read config from.
src/annotator/config.js:13:function configure(window_) {
src/annotator/config.js:14:  var config = {
src/annotator/config.js:19:  // Parse config from `<script class="js-hypothesis-config">` tags
src/annotator/config.js:21:    Object.assign(config, settings(window_.document));
src/annotator/config.js:23:    console.warn('Could not parse settings from js-hypothesis-config tags',
src/annotator/config.js:27:  // Parse config from `window.hypothesisConfig` function
src/annotator/config.js:28:  if (window_.hasOwnProperty('hypothesisConfig')) {
src/annotator/config.js:29:    if (typeof window_.hypothesisConfig === 'function') {
src/annotator/config.js:30:      Object.assign(config, window_.hypothesisConfig());
src/annotator/config.js:32:      throw new TypeError('hypothesisConfig must be a function, see: ' + docs);
src/annotator/config.js:36:  // Convert legacy keys/values in config to corresponding current
src/annotator/config.js:37:  // configuration.
src/annotator/config.js:38:  if (typeof config.showHighlights === 'boolean') {
src/annotator/config.js:39:    config.showHighlights = config.showHighlights ? 'always' : 'never';
src/annotator/config.js:44:  // The Chrome extension or proxy may already have provided this config
src/annotator/config.js:48:  // In environments where the config has not been injected into the DOM,
src/annotator/config.js:52:    Object.assign(config, directLinkedID);
src/annotator/config.js:54:  return config;
src/annotator/config.js:57:module.exports = configure;
src/annotator/main.js:3:var configure = require('./config');
src/annotator/main.js:41:var config = configure(window);
src/annotator/main.js:47:  if (config.hasOwnProperty('constructor')) {
src/annotator/main.js:48:    Klass = config.constructor;
src/annotator/main.js:49:    delete config.constructor;
src/annotator/main.js:52:  config.pluginClasses = pluginClasses;
src/annotator/main.js:54:  window.annotator = new Klass(document.body, config);
src/annotator/test/config-test.js:8:var configure = proxyquire('../config', {
src/annotator/test/config-test.js:23:describe('annotator.config', function() {
src/annotator/test/config-test.js:55:    it("returns the <link>'s href as config.app", function() {
src/annotator/test/config-test.js:56:      assert.equal(configure(window).app, link.href);
src/annotator/test/config-test.js:65:      assert.throws(function() { configure(window_); }, TypeError);
src/annotator/test/config-test.js:72:    configure(window_);
src/annotator/test/config-test.js:79:    it('reads the setting into the returned config', function() {
src/annotator/test/config-test.js:80:      // configure() just blindly adds any key: value settings that settings()
src/annotator/test/config-test.js:81:      // returns into the returned config object.
src/annotator/test/config-test.js:84:      var config = configure(fakeWindow());
src/annotator/test/config-test.js:86:      assert.equal(config.foo, 'bar');
src/annotator/test/config-test.js:96:      configure(fakeWindow());
src/annotator/test/config-test.js:100:      configure(fakeWindow());
src/annotator/test/config-test.js:106:  context("when there's a window.hypothesisConfig() function", function() {
src/annotator/test/config-test.js:107:    it('reads arbitrary settings from hypothesisConfig() into config', function() {
src/annotator/test/config-test.js:109:      window_.hypothesisConfig = sinon.stub().returns({foo: 'bar'});
src/annotator/test/config-test.js:111:      var config = configure(window_);
src/annotator/test/config-test.js:113:      assert.equal(config.foo, 'bar');
src/annotator/test/config-test.js:116:    specify('hypothesisConfig() settings override js-hypothesis-config ones', function() {
src/annotator/test/config-test.js:118:      window_.hypothesisConfig = sinon.stub().returns({
src/annotator/test/config-test.js:119:        foo: 'fooFromHypothesisConfigFunc'});
src/annotator/test/config-test.js:120:      fakeSettings.returns({foo: 'fooFromJSHypothesisConfigObj'});
src/annotator/test/config-test.js:122:      var config = configure(window_);
src/annotator/test/config-test.js:124:      assert.equal(config.foo, 'fooFromHypothesisConfigFunc');
src/annotator/test/config-test.js:127:    context('if hypothesisConfig() returns a non-object value', function() {
src/annotator/test/config-test.js:128:      it("doesn't add anything into the config", function() {
src/annotator/test/config-test.js:130:        window_.hypothesisConfig = sinon.stub().returns(42);
src/annotator/test/config-test.js:132:        var config = configure(window_);
src/annotator/test/config-test.js:134:        delete config.app; // We don't care about config.app for this test.
src/annotator/test/config-test.js:135:        assert.deepEqual({}, config);
src/annotator/test/config-test.js:140:  context("when window.hypothesisConfig() isn't a function", function() {
src/annotator/test/config-test.js:143:      window_.hypothesisConfig = 'notAFunction';
src/annotator/test/config-test.js:146:        function() { configure(window_); }, TypeError,
src/annotator/test/config-test.js:147:        'hypothesisConfig must be a function, see: https://h.readthedocs.io/en/latest/embedding.html'
src/annotator/test/config-test.js:165:      // returned config, unmodified.
src/annotator/test/config-test.js:175:        var config = configure(fakeWindow());
src/annotator/test/config-test.js:177:        assert.equal(config.showHighlights, test.out);
src/annotator/test/config-test.js:183:    configure(fakeWindow());
src/annotator/test/config-test.js:197:    it('blindly adds the properties of the object to the config', function() {
src/annotator/test/config-test.js:198:      assert.equal(configure(fakeWindow()).foo, 'bar');
src/annotator/test/config-test.js:203:      // settings() or from window.hypothesisConfig().
src/annotator/test/config-test.js:209:      window_.hypothesisConfig = sinon.stub().returns({
src/annotator/test/config-test.js:210:        foo: 'fromHypothesisConfig',
src/annotator/test/config-test.js:213:      assert.equal(configure(window_).foo, 'fromExtractAnnotationQuery');
src/annotator/test/integration/anchoring-test.js:49:  var guestConfig;
src/annotator/test/integration/anchoring-test.js:53:    guestConfig = {pluginClasses: {CrossFrame: FakeCrossFrame}};
src/annotator/test/integration/anchoring-test.js:60:    guest = new Guest(container, guestConfig);
src/annotator/delegator.coffee:33:# config - An Object literal of config settings.
src/annotator/delegator.coffee:43:  constructor: (element, config) ->
src/annotator/delegator.coffee:44:    @options = $.extend(true, {}, @options, config)
src/annotator/guest.coffee:58:  constructor: (element, config) ->
src/annotator/host.coffee:6:  constructor: (element, config) ->
src/annotator/host.coffee:8:    # Some config settings are not JSON-stringifiable (e.g. JavaScript
src/annotator/host.coffee:9:    # functions) and will be omitted when the config is JSON-stringified.
src/annotator/host.coffee:12:    if config.services?[0]
src/annotator/host.coffee:13:      service = config.services[0]
src/annotator/host.coffee:25:    # Make a copy of all config settings except `config.app`, the app base URL,
src/annotator/host.coffee:26:    # and `config.pluginClasses`
src/annotator/host.coffee:27:    configParam = 'config=' + encodeURIComponent(
src/annotator/host.coffee:28:      JSON.stringify(Object.assign({}, config, {app:undefined, pluginClasses: undefined }))
src/annotator/host.coffee:30:    if config.app and '?' in config.app
src/annotator/host.coffee:31:      config.app += '&' + configParam
src/annotator/host.coffee:33:      config.app += '?' + configParam
src/annotator/host.coffee:41:    .attr('src', config.app)
src/annotator/host.coffee:55:      if config.showHighlights == undefined
src/annotator/host.coffee:57:        config.showHighlights = 'always'
src/annotator/host.coffee:58:      this.setVisibleHighlights(config.showHighlights == 'always')
src/annotator/sidebar.coffee:26:  constructor: (element, config) ->
src/annotator/sidebar.coffee:30:    if config.openSidebar || config.annotations || config.query
src/annotator/sidebar.coffee:40:    serviceConfig = config.services?[0]
src/annotator/sidebar.coffee:41:    if serviceConfig
src/annotator/sidebar.coffee:42:      @onLoginRequest = serviceConfig.onLoginRequest
src/annotator/sidebar.coffee:43:      @onLogoutRequest = serviceConfig.onLogoutRequest
src/annotator/sidebar.coffee:44:      @onSignupRequest = serviceConfig.onSignupRequest
src/annotator/sidebar.coffee:45:      @onProfileRequest = serviceConfig.onProfileRequest
src/annotator/sidebar.coffee:46:      @onHelpRequest = serviceConfig.onHelpRequest
src/annotator/test/guest-test.coffee:53:  guestConfig = null
src/annotator/test/guest-test.coffee:55:  createGuest = (config={}) ->
src/annotator/test/guest-test.coffee:56:    config = Object.assign({}, guestConfig, config)
src/annotator/test/guest-test.coffee:58:    return new Guest(element, config)
src/annotator/test/guest-test.coffee:67:    guestConfig = {pluginClasses: {}}
src/annotator/test/guest-test.coffee:92:    guestConfig.pluginClasses['CrossFrame'] = CrossFrame
src/annotator/test/guest-test.coffee:103:      guestConfig.pluginClasses['FakePlugin'] = FakePlugin
src/annotator/test/host-test.coffee:8:  hostConfig = {pluginClasses: {}}
src/annotator/test/host-test.coffee:10:  createHost = (config={}, element=null) ->
src/annotator/test/host-test.coffee:11:    config = Object.assign({app: '/base/annotator/test/empty.html'}, hostConfig, config)
src/annotator/test/host-test.coffee:14:    return new Host(element, config)
src/annotator/test/host-test.coffee:27:    hostConfig.pluginClasses['CrossFrame'] = CrossFrame
src/annotator/test/host-test.coffee:70:  describe 'config', ->
src/annotator/test/host-test.coffee:85:    it 'passes config to the sidebar iframe', ->
src/annotator/test/host-test.coffee:88:      configStr = encodeURIComponent(JSON.stringify({annotations: '1234'}))
src/annotator/test/host-test.coffee:89:      assert.equal(host.frame[0].children[0].src, appURL + '?config=' + configStr)
src/annotator/test/sidebar-test.coffee:10:  sidebarConfig = {pluginClasses: {}}
src/annotator/test/sidebar-test.coffee:12:  createSidebar = (config={}) ->
src/annotator/test/sidebar-test.coffee:13:    config = Object.assign({}, sidebarConfig, config)
src/annotator/test/sidebar-test.coffee:15:    return new Sidebar(element, config)
src/annotator/test/sidebar-test.coffee:26:    sidebarConfig.pluginClasses['CrossFrame'] = CrossFrame
src/annotator/test/sidebar-test.coffee:52:        sidebar = createSidebar(config={services: [{onLoginRequest: onLoginRequest}]})
src/annotator/test/sidebar-test.coffee:59:        # Even though config.services is an array it only calls the onLoginRequest
src/annotator/test/sidebar-test.coffee:66:          config={
src/annotator/test/sidebar-test.coffee:87:          config={
src/annotator/test/sidebar-test.coffee:102:        sidebar = createSidebar(config={})  # No config.services
src/annotator/test/sidebar-test.coffee:106:        sidebar = createSidebar(config={services: []})
src/annotator/test/sidebar-test.coffee:110:        sidebar = createSidebar(config={services: [{}]})
src/annotator/test/sidebar-test.coffee:116:        sidebar = createSidebar(config={services: [{onLogoutRequest: onLogoutRequest}]})
src/annotator/test/sidebar-test.coffee:125:        sidebar = createSidebar(config={services: [{onSignupRequest: onSignupRequest}]})
src/annotator/test/sidebar-test.coffee:134:        sidebar = createSidebar(config={services: [{onProfileRequest: onProfileRequest}]})
src/annotator/test/sidebar-test.coffee:143:        sidebar = createSidebar(config={services: [{onHelpRequest: onHelpRequest}]})

seanh · 2017-05-26T10:30:47Z

And these are the various options objects in src/annotator that I didn't rename because I don't think they refer to the main options (now config) object:

$ ag options src/annotator/**.{js,coffee}
src/annotator/adder.js:98: * @param {Object} options - Options object specifying `onAnnotate` and `onHighlight`
src/annotator/adder.js:101:function Adder(container, options) {
src/annotator/adder.js:139:      options.onAnnotate();
src/annotator/adder.js:141:      options.onHighlight();
src/annotator/anchoring/test/fake-pdf-viewer-application.js:6: * @typedef {Object} Options
src/annotator/anchoring/test/fake-pdf-viewer-application.js:19: * @param {Options} options
src/annotator/anchoring/test/fake-pdf-viewer-application.js:21:function FakePDFViewerApplication(options) {
src/annotator/anchoring/test/fake-pdf-viewer-application.js:23:    if (index < 0 || index >= options.content.length) {
src/annotator/anchoring/test/fake-pdf-viewer-application.js:29:  this._content = options.content;
src/annotator/anchoring/test/fake-pdf-viewer-application.js:30:  this._container = options.container;
src/annotator/anchoring/test/fake-pdf-viewer-application.js:36:    pagesCount: options.content.length,
src/annotator/anchoring/test/fake-pdf-viewer-application.js:61:        items: options.content[index].split(/\n/).map(function (line) {
src/annotator/annotation-sync.js:8:function AnnotationSync(bridge, options) {
src/annotator/annotation-sync.js:13:  if (!options.on) {
src/annotator/annotation-sync.js:14:    throw new Error('options.on unspecified for AnnotationSync.');
src/annotator/annotation-sync.js:17:  if (!options.emit) {
src/annotator/annotation-sync.js:18:    throw new Error('options.emit unspecified for AnnotationSync.');
src/annotator/annotation-sync.js:23:  this._on = options.on;
src/annotator/annotation-sync.js:24:  this._emit = options.emit;
src/annotator/test/annotation-sync-test.js:10:  var options;
src/annotator/test/annotation-sync-test.js:19:      return new AnnotationSync(fakeBridge, options);
src/annotator/test/annotation-sync-test.js:31:    options = {
src/annotator/test/annotation-sync-test.js:53:        options.on('annotationDeleted', eventStub);
src/annotator/test/annotation-sync-test.js:77:        options.emit = function() { assert(!annSync.cache.tag1); };
src/annotator/test/annotation-sync-test.js:106:        options.on('annotationsLoaded', loadedStub);
src/annotator/test/annotation-sync-test.js:120:        options.emit('beforeAnnotationCreated', ann);
src/annotator/test/annotation-sync-test.js:133:          options.emit('beforeAnnotationCreated', ann);
src/annotator/anchoring/html.coffee:9:querySelector = (type, root, selector, options) ->
src/annotator/anchoring/html.coffee:12:      anchor = type.fromSelector(root, selector, options)
src/annotator/anchoring/html.coffee:13:      range = anchor.toRange(options)
src/annotator/anchoring/html.coffee:29:# :param Object options: Options to pass to the anchor implementations.
src/annotator/anchoring/html.coffee:33:exports.anchor = (root, selectors, options = {}) ->
src/annotator/anchoring/html.coffee:47:        options.hint = position.start  # TextQuoteAnchor hint
src/annotator/anchoring/html.coffee:66:      return querySelector(FragmentAnchor, root, fragment, options)
src/annotator/anchoring/html.coffee:71:      return querySelector(RangeAnchor, root, range, options)
src/annotator/anchoring/html.coffee:76:      return querySelector(TextPositionAnchor, root, position, options)
src/annotator/anchoring/html.coffee:82:      return querySelector(TextQuoteAnchor, root, quote, options)
src/annotator/anchoring/html.coffee:87:exports.describe = (root, range, options = {}) ->
src/annotator/anchoring/html.coffee:92:      anchor = type.fromRange(root, range, options)
src/annotator/anchoring/html.coffee:93:      selector = anchor.toSelector(options)
src/annotator/anchoring/pdf.coffee:113:anchorByPosition = (page, anchor, options) ->
src/annotator/anchoring/pdf.coffee:118:    selector = anchor.toSelector(options)
src/annotator/anchoring/pdf.coffee:206:# :param Object options: Options to pass to the anchor implementations.
src/annotator/anchoring/pdf.coffee:210:exports.anchor = (root, selectors, options = {}) ->
src/annotator/anchoring/pdf.coffee:243:        return anchorByPosition(page, anchor, options)
src/annotator/anchoring/pdf.coffee:249:        return anchorByPosition(page, anchor, options)
src/annotator/anchoring/pdf.coffee:267:# :param Object options: Options passed to `TextQuoteAnchor` and
src/annotator/anchoring/pdf.coffee:270:exports.describe = (root, range, options = {}) ->
src/annotator/anchoring/pdf.coffee:297:    position = new TextPositionAnchor(root, start, end).toSelector(options)
src/annotator/anchoring/pdf.coffee:303:    quote = TextQuoteAnchor.fromRange(root, r, options).toSelector(options)
src/annotator/anchoring/types.coffee:50:  toSelector: (options = {}) ->
src/annotator/anchoring/types.coffee:51:    range = @range.serialize(@root, options.ignoreSelector)
src/annotator/anchoring/types.coffee:95:  @fromRange: (root, range, options) ->
src/annotator/anchoring/types.coffee:96:    selector = domAnchorTextQuote.fromRange(root, range, options)
src/annotator/anchoring/types.coffee:111:  toRange: (options = {}) ->
src/annotator/anchoring/types.coffee:112:    range = domAnchorTextQuote.toRange(@root, this.toSelector(), options)
src/annotator/anchoring/types.coffee:117:  toPositionAnchor: (options = {}) ->
src/annotator/anchoring/types.coffee:118:    anchor = domAnchorTextQuote.toTextPosition(@root, this.toSelector(), options)
src/annotator/delegator.coffee:16:# from. It provides basic functionality such as instance options, event
src/annotator/delegator.coffee:23:# Public: Options object. Extended on initialisation.
src/annotator/delegator.coffee:24:  options: {}
src/annotator/delegator.coffee:30:# hash and extends the @options object.
src/annotator/delegator.coffee:44:    @options = $.extend(true, {}, @options, config)
src/annotator/delegator.coffee:72:#   @options = {"click": "clickedElement"}
src/annotator/delegator.coffee:76:#   @options = {"form submit": "submitForm"}
src/annotator/delegator.coffee:82:#   @options = {"annotation:save": "updateAnnotationStore"}
src/annotator/guest.coffee:43:  options:
src/annotator/guest.coffee:83:    cfOptions =
src/annotator/guest.coffee:89:    this.addPlugin('CrossFrame', cfOptions)
src/annotator/guest.coffee:97:    for own name, opts of @options
src/annotator/guest.coffee:98:      if not @plugins[name] and @options.pluginClasses[name]
src/annotator/guest.coffee:101:  addPlugin: (name, options) ->
src/annotator/guest.coffee:105:      klass = @options.pluginClasses[name]
src/annotator/guest.coffee:107:        @plugins[name] = new klass(@element[0], options)
src/annotator/guest.coffee:213:      options = {
src/annotator/guest.coffee:217:      return self.anchoring.anchor(root, target.selector, options)
src/annotator/guest.coffee:311:      options = {
src/annotator/guest.coffee:316:      return self.anchoring.describe(root, range, options)
src/annotator/pdf-sidebar.coffee:5:  options:
src/annotator/plugin.coffee:4:  constructor: (element, options) ->
src/annotator/plugin/bucket-bar.coffee:52:  # Plugin options
src/annotator/plugin/bucket-bar.coffee:53:  options:
src/annotator/plugin/bucket-bar.coffee:71:  constructor: (element, options) ->
src/annotator/plugin/bucket-bar.coffee:72:    super $(@html), options
src/annotator/plugin/bucket-bar.coffee:74:    if @options.container?
src/annotator/plugin/bucket-bar.coffee:75:      $(@options.container).append @element
src/annotator/plugin/bucket-bar.coffee:82:    for scrollable in @options.scrollables ? []
src/annotator/plugin/bucket-bar.coffee:88:    for scrollable in @options.scrollables ? []
src/annotator/plugin/bucket-bar.coffee:166:        x - index[index.length-1] > @options.gapSize      # A large gap?
src/annotator/plugin/cross-frame.coffee:20:  constructor: (elem, options) ->
src/annotator/plugin/cross-frame.coffee:23:    opts = extract(options, 'server')
src/annotator/plugin/cross-frame.coffee:28:    opts = extract(options, 'on', 'emit')
src/annotator/plugin/test/bucket-bar-test.coffee:4:  createBucketBar = (options) ->
src/annotator/plugin/test/bucket-bar-test.coffee:6:    new BucketBar(element, options || {})
src/annotator/plugin/test/cross-frame-test.coffee:17:  createCrossFrame = (options) ->
src/annotator/plugin/test/cross-frame-test.coffee:22:    return new CrossFrame(element, $.extend({}, defaults, options))
src/annotator/plugin/test/cross-frame-test.coffee:59:    it 'passes the options along to the bridge', ->
src/annotator/plugin/test/cross-frame-test.coffee:71:    it 'passes along options to AnnotationSync', ->
src/annotator/plugin/toolbar.coffee:25:    if @options.container?
src/annotator/plugin/toolbar.coffee:26:      $(@options.container).append @toolbar
src/annotator/sidebar.coffee:15:  options:
src/annotator/sidebar.coffee:193:    if @options.showHighlights == 'whenSidebarOpen'
src/annotator/sidebar.coffee:205:    if @options.showHighlights == 'whenSidebarOpen'
src/annotator/test/guest-test.coffee:126:      options = CrossFrame.lastCall.args[1]
src/annotator/test/guest-test.coffee:127:      assert.isFunction(options.on)
src/annotator/test/guest-test.coffee:128:      assert.isFunction(options.emit)
src/annotator/test/guest-test.coffee:138:      options = null
src/annotator/test/guest-test.coffee:143:        options = CrossFrame.lastCall.args[1]
src/annotator/test/guest-test.coffee:149:        options.on('foo', fooHandler)
src/annotator/test/guest-test.coffee:150:        options.on('bar', barHandler)
src/annotator/test/guest-test.coffee:159:      options = null
src/annotator/test/guest-test.coffee:164:        options = CrossFrame.lastCall.args[1]
src/annotator/test/guest-test.coffee:169:        options.emit('annotationDeleted', ann)
src/annotator/test/guest-test.coffee:177:        options.emit('annotationsLoaded', [ann1, ann2])
src/annotator/test/guest-test.coffee:189:        options.emit('foo', '1', '2')
src/annotator/test/guest-test.coffee:190:        options.emit('bar', '1', '2')

Notice this one in sidebar.coffee:

src/annotator/sidebar.coffee
15:  options:
193:    if @options.showHighlights == 'whenSidebarOpen'
205:    if @options.showHighlights == 'whenSidebarOpen'

It looks like something merges at least some of the settings from the main options (config) object (showHighlights, app, openSidebar, maybe it's all of them) into Sidebar's own separate @options object that contains other objects like BucketBar, Document, TextSelection, Toolbar. On master, in sidebar.coffee, options is the main options obj and @options is a separate obj (and there is one line in which @options is called options and then somewhere (not in sidebar.coffee itself) settings from options get merged into @options!

Anyway, I decided not to rename @options.

seanh · 2017-05-26T10:43:15Z

and then somewhere (not in sidebar.coffee itself) settings from options get merged into @options!

Sidebar extends Host which extends Guest which extends Delegator and it's in delegator.coffee that the two options objects get merged together (which on this branch is now merging config into @options):

client/src/annotator/delegator.coffee

Line 44 in e76407f

@options = $.extend(true, {}, @options, config)

For our sanity this probably shouldn't be happening but I'll leave it as it is for this PR, at least the two objects are now called options and config not options and options, I'm trying to refactor the config code right now not the Sidebar/Host/Guest/Delegator code.

robertknight · 2017-05-31T11:00:45Z

src/annotator/config.js

@@ -10,15 +10,15 @@ var docs = 'https://h.readthedocs.io/en/latest/embedding.html';
 *
 * @param {Window} window_ - The Window object to read config from.
 */
-function config(window_) {
-  var options = {
+function configure(window_) {


I think this name is a mistake. configure is a verb, which implies that it modifies something or applies the configuration. All that is actually happening here is that the configuration is being read from the page. readConfig would be fine.

I had a look through the "h" code and indeed, most of the functions that have configure prefix do actually modify some aspect of the global environment or a passed in object, so the use of a verb is appropriate there.

robertknight

This is fine except that I think calling the function that reads settings from the page configure is a mistake because that implies to me that it modifies the global environment in some way. I'd be fine with calling it readConfig or something like that.

robertknight · 2017-05-31T11:42:42Z

Sidebar extends Host which extends Guest which extends Delegator and it's in delegator.coffee
that the two options objects get merged together (which on this branch is now merging config
into @options

Right, well the real problem here is the depth of the inheritance hierarchy. I think it should be something much more like Delegator => {Sidebar, Host, Guest} where "Delegator" (or whatever it ends up being called) is the event bus and then the other components compose the pieces they need to fulfil their role.

Rename the `config()` function that creates the `options` object to `configFrom()`. This is because I want to rename the `options` object to `config` in a future commit, so I need the `config()` function to be called something else. I think `configFrom(window)` is also clearer (that this is a function that returns a `config` object created from the given window) than `config(window)` (which could be read as configuring the window, for example).

This makes it easier to grep for uses of this function.

There are two reasons for renaming the `options` object to `config`: **First**, it's more consistent. The file that creates this object is called `config.js`, and the function that creates the object is called `configFrom()`, and yet the object ends up getting called `options` (but you have to look in another file, `main.js`, to find this out). `config` is also used elsewhere as the name for the main configuration object, for example in Pyramid/h, in the client docs ("Configuring the Client", "configuration settings") and public API (`class="js-hypothesis-config"` scripts and `window.hypothesisConfig()` functions), etc. These "options" that the `src/annotator/` code reads from the host page also end up getting renamed to `hostPageConfig` when they get passed over in to the `src/sidebar/` code. **Second**, it's more unique. There are a number of other objects in the `src/annotator/` code that are called options, sometimes the main options object and another local options object are even used in the same function. There's no other objects called config.

seanh · 2017-05-31T16:36:44Z

I went with configFrom() for the function name.

seanh added the Needs Review label May 25, 2017

seanh force-pushed the rename-options-to-config-in-annotator branch from e291949 to 6ba3e48 Compare May 25, 2017 19:34

seanh self-assigned this May 30, 2017

seanh mentioned this pull request May 30, 2017

Client config is complicated hypothesis/product-backlog#299

Open

robertknight reviewed May 31, 2017

View reviewed changes

robertknight approved these changes May 31, 2017

View reviewed changes

seanh added 4 commits May 31, 2017 17:26

Use configFrom() as configFrom()

ec6c8cc

This makes it easier to grep for uses of this function.

Fix a comment

4ecd8cc

seanh force-pushed the rename-options-to-config-in-annotator branch from e76407f to 4ecd8cc Compare May 31, 2017 16:36

seanh merged commit 6ba3b75 into master May 31, 2017

seanh deleted the rename-options-to-config-in-annotator branch May 31, 2017 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename options to config in annotator #404

Rename options to config in annotator #404

seanh commented May 25, 2017 •

edited

Loading

seanh commented May 25, 2017 •

edited

Loading

codecov-io commented May 25, 2017 •

edited

Loading

seanh commented May 26, 2017

seanh commented May 26, 2017 •

edited

Loading

seanh commented May 26, 2017 •

edited

Loading

robertknight May 31, 2017 •

edited

Loading

robertknight May 31, 2017

robertknight left a comment

robertknight commented May 31, 2017

seanh commented May 31, 2017

Rename options to config in annotator #404

Rename options to config in annotator #404

Conversation

seanh commented May 25, 2017 • edited Loading

seanh commented May 25, 2017 • edited Loading

codecov-io commented May 25, 2017 • edited Loading

Codecov Report

seanh commented May 26, 2017

seanh commented May 26, 2017 • edited Loading

seanh commented May 26, 2017 • edited Loading

robertknight May 31, 2017 • edited Loading

Choose a reason for hiding this comment

robertknight May 31, 2017

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

robertknight commented May 31, 2017

seanh commented May 31, 2017

seanh commented May 25, 2017 •

edited

Loading

seanh commented May 25, 2017 •

edited

Loading

codecov-io commented May 25, 2017 •

edited

Loading

seanh commented May 26, 2017 •

edited

Loading

seanh commented May 26, 2017 •

edited

Loading

robertknight May 31, 2017 •

edited

Loading