fix: update gene symbol when it is `None` and `AssemblyMapper` normalizer

[markgene]

Two fixes:

1. The _update_gene_symbol of VariantMapper class did not update symbol when it is None.

2. The normalizer of AssemblyMapper uses a VariantMapper configured inconsistently from AssemblyMapper itself. The AssemblyMapper._norm is a Normalizer object. Normalizer has a field vm of VariantMapper class. In the current code, when initializing an AssemblyMapper object with the argument of a different value from global_config (e.g. add_gene_symbol=True, the vm will be created using global_config, rather than the parameters passed to AssemblyMapper. This is confusing, because the mappers are configured differently and can bring unexpected behaviors.

The fix passes a VariantMapper object initialized using the same arguments as AssemblyMapper object.

[github-actions[bot]]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[davmlaw]

Hi, thanks for the pull request, here are some comments on the change:

1. _update_gene_symbol

The existing code:

if not symbol:

Already handles symbol being None - I verified this:

In [16]: var_c.gene = None

In [17]: vm._update_gene_symbol(var_c, "test_gene_symbol")  # existing code
Out[17]: SequenceVariant(ac=NM_002386.3, type=c, posedit=-5_1del, gene=test_gene_symbol)

In [18]: var_c.gene
Out[18]: 'test_gene_symbol'

Next time - it's good to include a test that fails on the old code, and then passes on the new code (eg an automated test doing what I did above). I think if you did this, you would have discovered that the code change was unnecessary

2. Given an AssemblyMapper is a VariantMapper, could you pass variantmapper=self? That would future proof the code so that if we added some new parameters, we wouldn't have to make sure we update them in 2 places

Also - is this a fix for issue #704 - if so, could you please link issue in pull request?

Also, a test would be nice (eg recreate #704 on old code, validate it's fixed in new code)

Thanks

[github-actions[bot]]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions[bot]]

This PR was closed because it has been stalled for 7 days with no activity.

[davmlaw]

@markgene - thanks for your contribution, I have taken your change, removed the gene parts, and added a unit test - submitted as a new pull request: https://github.com/biocommons/hgvs/pull/746