I don't like this much. You have a parameter that is not really exposed.
Also please use a regexp as this:

int(re.split('(Stimulus|Response)/[RS]\s*', "Response/R 1")[-1])

I am not it has the full logic but it's much more robust and easier to understand what pattern to match

Should I just remove add_for_r from the call signature?

Otherwise I changed it to this, is that what you mean:

def _bv_parser(description, add_for_r=1000):
    """Parse BrainVision event codes (like `Stimulus/S 11`) to ints."""
    try:
        *_, kind, code = re.split('(Stimulus|Response)/[RS]\s*', description)
        code = int(code)
        if kind == "Response":
            code += add_for_r
        return code
    except ValueError:  # probably not coercable to int, or no match at all
        warn("Marker '%s' could not be unambiguously mapped to an int and "
             "will be dropped." % description)

can you use rather re.match rather than a try-catch?
but yes this for me really cleaner.

@larsoner also see here

Events not matching the pattern should probably not be dropped but assigned arbitrary integers (just like we did before). This will fix the current error in the test.

we had that at some point and we reverted. (we can do it again)

I think we have to, because we want to have all events, not just the ones matching the patterns.

How should we do that? It must be deterministic, mapping the same code to the same int every time. We could do it externally, or hash it somehow, or use a decorator?

Good question. Maybe sort the rest alphabetically and assign numbers starting with 2000?

If we want to go that route I would put all these things in a function and register the default behaviors there.

event_id = get_auto_event_id(raw.__class__()) if event_id == 'auto' else event_id

then remove None from the parameters and return the default behaviour if raw does not quack properly. (which is what is happening here: https://github.com/mne-tools/mne-python/blob/master/mne/annotations.py#L778)

it would be something like this:

class RawBrainVision(BaseRaw):

    @classmethod
    def auto_event_id(cls):
        return _bv_parse

...

def get_auto_event_id(kls):
    foo = kls.auto_event_id if 'auto_event_id' in kls.__dict__.keys() else _Counter()
    return foo

(plus we should check that the parameter event_id is only is a dict, a callable or a string strictly equal to 'auto')

Yes, this can be refactored to make it nicer, but it's imperative it gets in at .18.

I agree that getting this done for 0.18 should be our priority for now. @massich you could also add the classmethod to the base class and return a counter function there, then you don't even have to write that if clause.

we don't need to parse the file to make sure that the parsing works.

I would rather like to see a battery of falling edge cases than the expected.

what would happen if I get something like 'stiMOulus/S', 'Stimulus\S', 'foobar/09876564578' ?

This is not how the Brainvision recorder works (... in principle one could set up pycorder to work like that I guess? But to do that one needs to be sufficiently python savy to quickly come up with a parser)

Again, I'm fine with suggestions for alternatives, but it must get in by .18.

I mean, that we should make sure that the else branch of this line https://github.com/mne-tools/mne-python/pull/6276/files#diff-db95532aad4fae1b22f34cbf50ed7f19R845 does not generate unexpected behaviors.

As far as I know the recorder, these events will never be interesting for epoching; i.e., it's always NewSegment or something like that.

Right @cbrnr ?

Yes, these are not important for epoching, but we still need to catch them.

So I'm +1 for adding failing test cases as @massich suggests. Like I said, I think we should return all events not just the ones relevant for epoching (users can and should filter out events relevant for them after we return everything, which will be easy because they can filter by value, i.e. between 0-255 and 1000-1255 if they only want stimulus and response markers).

This is going to be way too long / hard to parse. I'd do:

    event_id : dict | callable | None | 'auto'
        Can be:

        - **dict**: string keys mapping to and integer values as used in mne.Epochs
          to map annotation descriptions to integer event codes. Only the
          keys present will be mapped and the annotations with other descriptions
          will be ignored.
        - **callable**: takes a string input, and outputs an integer to use, or None
          to ignore.
        - **None**: All descriptions of annotations are mapped and assigned
          unique integer values starting at 1 based on the ``sorted`` annotation
          descriptions.
        - **'auto'**: use an appropriate parser if the raw instance corresponds to:

          - Brainvision annotations: stimulus events stripped to their integer part;
            Response events mapped to their integer part + 1000; all other events
            dropped.
          - All others: Behaves like None.

Looks good to me.

Don't drop other events please.

just for the record, this function has no return value. We should try to avoid multiple return points. (or make sure that all branches do return)