Can you also check the tinybuild and add tests that fail in master and work now? It ensures that the real-world / end use cases improve in addition to our lower-level functions.

There might already be examples that didn't work before but now work (i.e., just change test_full.py but not anything in the tinybuild/ directory itself), which would be great. But if you need to also change things in tinybuild/ to get something that fails in master but works here it's fine, too.

There isn't an appropriate import example in tinybuild/ - I can add a import a.b to /plot_numpy_matplotlib ('Link to other packages') example and add an assertion somewhere in test_full. Does that work?

Yes that works, but in addition to what you've done you should add something like the following after line ~198 in test_full.py as that's where we test that the code blocks have the appropriate outgoing links:

assert 'sphinx_gallery.backreferences.identify_names.html' in lines

It's fine to have all this in plot_numpy_matplotlib.py of tinybuild, no need for a separate file.

This break will always occur in the first iteration of the for loop, so do we even need the loop? It seems likely that this loop is now equivalent to just doing:

if len(module) >= 1:  # probably don't even need this conditional
    full_name = ...
    yield ...

Is the loop no longer necessary because somewhere else we're already traversing the full_name.split('.') levels backward?

Yes you are right. The loop is needed for the conditional if local_name in self.imported_names: but is not required for the condition elif local_name in self.global_variables:.

In the case where you are calling an indirectly imported function (or class), e.g.,

import h.i
h.i.j()

the local_name, h, will not be in self.imported_names or self.global_variables. It needs to traverse the loop for the first conditional if local_name in self.imported_names:.

Not sure how to fix...

It needs to traverse the loop for the first conditional if local_name in self.imported_names:

I'm confused :( Which loop are you talking about with this statement? The one up on line 69, or 80? Those are the two that satisfy if local_name in self.imported_names. Maybe the best thing to do is to add a commit with some code comments to help explain why some of the conditionals / loops are needed in the places that you know they are (and even # XXX don't know why this is needed in some places).

I'm just saying that the change you made in the code is equivalent to something simpler. So we should either simplify the loop starting on line 108 accordingly or revert the addition of the break.

Are you saying that (regardless of this equivalence) the code as it's written does not work properly?

I added a comment but not sure if it helps.

I was talking about the 69 for loop for split_level in range(max_import_splits):

And yes I realise now that the break was on the wrong level. It was meant to be similar to/on the same level as the break here:

            for split_level in range(max_import_splits):
                local_name_split = name.split('.')
                local_name = '.'.join(local_name_split[:split_level+1])
                remainder = name[len(local_name):]
                class_attr = False
                if local_name in self.imported_names:
                    # Join import path to relative path
                    full_name = self.imported_names[local_name] + remainder
                    if local_name in self.global_variables:
                        obj = self.global_variables[local_name]
                        if remainder:
                            for level in remainder[1:].split('.'):
                                obj = getattr(obj, level)
                        is_class = inspect.isclass(obj)
                    else:
                        is_class = False
                    yield name, full_name, class_attr, is_class
                    break   # <----this break!!

but you made me realise that the for split_level in range(max_import_splits): loop is only required for the first condition. i.e.

for split_level in range(max_import_splits):
    if local_name in self.imported_names:
    <the for loop is needed in here>
    elif local_name in self.global_variables:
    <the for loop isn't needed here>

This suggests that the code should be refactored to:

if local_name in self.imported_names:
    <the for loop is needed in here>
    for split_level in range(max_import_splits):
        ...
elif local_name in self.global_variables:
    <the for loop isn't needed here>

Right?

Never mind, this wouldn't work as local_name is formed using the split_level. So maybe you were talking about a different for loop...

Different loop?

For case of import h.i; h.i.j():

for split_level in range(max_import_splits):
    if local_name in self.imported_names:
        # the for loop is needed in here because self.imported_names is `h.i`
        # so the loop would check for `h` in self.imported_names first 
        # time round,  `h.i` in imported_names
    elif local_name in self.global_variables:
        # you don't need the loop here as you are checking for 
        # global variable names, which
        # won't have a '.' (i think?)

But I don't have a great understanding of what goes on after the elif local_name in self.global_variables: so please correct me.

I've put the break at the correct level now - which may or may not make things clearer.

The break broke things because it seems that you need to traverse the loop again if during the first time round the loop you have this case:

for split_level in range(max_import_splits):
    if local_name in self.imported_names:
        # condition not met on first loop
    elif local_name in self.global_variables:
        # condition met on first loop

Because otherwise example_code_obj returned from identify_names is incorrect. I don't really understand why.
e.g., for tinybuild sphinx_gallery.backreferences.identify_names([('text', 'Text block', 1)]), example_code_obj is:

# with break
('sphinx_gallery.backreferences.identify_names', {'name': 'backreferences.identify_names', 'module': 'builtins.module', 'module_short': 'builtins.module', 'is_class': False})])
# without break
('sphinx_gallery.backreferences.identify_names', {'name': 'identify_names', 'module': 'sphinx_gallery.backreferences', 'module_short': 'sphinx_gallery.backreferences', 'is_class': False}),

I think that without the break both are yielded by get_mapping but somehow identify_names knows that 'name': 'backreferences.identify_names' is the good one so only returns this? I'm a bit confused by how identify_names works but does that make sense?

but somehow identify_names knows that 'name': 'backreferences.identify_names' is the good one

It looks for valid intersphinx references, so yeah it should subselect the correct one (wherever it's documented in the docs)

Yes that works, but in addition to what you've done you should add something like the following after line ~198 in test_full.py as that's where we test that the code blocks have the appropriate outgoing links:

assert 'sphinx_gallery.backreferences.identify_names.html' in lines

It's fine to have all this in plot_numpy_matplotlib.py of tinybuild, no need for a separate file.