lazr.config pop() has surprising behaviour

[Curtis Hovey]

Calling config.pop('config_data_name') will remove and return the named config, and all the config_data above it in the overlays stack. This is not what most developers would expect to happen when using pop(). The behaviour is valuable, but not right for the name 'pop'. I suggest:

    rename pop('config_data_name') => popByName('config_data_name')

The add a pop() method that works are expected:

    add pop()

Which removes and returns the top config_data on the overlays stack.

[Curtis Hovey (sinzui)]

This behaviour should be fixed before other parties use lazr.config.

[James Henstridge (jamesh)]

As mentioned in email, I think we still want to pass a config name for the pop() method we recommend people use. In the following sequence of events, I want an error at (3):

  1. push overlay1
  2. push overlay2
  3. pop overlay1
  4. pop overlay2

Without the config name, we can't distinguish this kind of error from correct usage of the API.

[Curtis Hovey (sinzui)]

I agree that seeing a error at 3 would be best.

Instead of popByName(), I think we want a slightly different behaviour from the current pop(). I think we want a restore(config_data_name), which makes the named ConfigData the top of the stack, the items that were above it are lost. This feature makes it easier to restore the config to a known state. There are two cases for this:

    1. A layer pushes a marker config onto the overlays so that it can be
        restored for the next test.
        layer -> config.push('pagetestlayer', '')
         test -> config.push('test data', test_data)
         test -> config.push('test data 2', test_data_2)
        layer -> config.restore('pagetestlayer')

    2. A test saves the name of the top config_data to restore it.
        starting_state = config.overlays[0].name
        ... lots of config.push() calls
        config.restore(starting_state)

[Jonathan Lange (jml)]

I'd rather see case 2 handled using addCleanup:

    config.push('test data', test_data)
    self.addCleanup(config.pop, 'test_data')

In fact, we could even add a helper method to do this:

    def pushConfig(self, name, value):
        config.push(name, value)
        self.addCleanup(config.pop, name)

[Jonathan Lange (jml)]

Actually, would 'patch' and 'unpatch' be better names, rather than 'push' and 'pop', which imply a stack-like structure that isn't strictly there?

[Jonathan Lange (jml)]

applyPatch/unapplyPatch as synonyms.