change LineString facecolor to None

[smartlixx]

Rationale

close #1782 by changing LineString geometries facecolor to None.

Implications

[greglucas]

What about updating the styler here? You can pass a callable that updates the color that way. You might be able to put the isinstance check in that way and it would act on each geometry then.

[smartlixx]

OK. But maybe it should be clarified that we only do styler update for LineString when
(1) styler is not set (None); or
(2) facecolor is not set, otherwise we should assume the user wants to fill the LineString by explicitly specifying facecolor.

And, what if user passes a callable to styler?

[greglucas]

I agree with all of that. If a user passes a callable for styler then this should have no effect. You could probably add an if callable(styler): return styler in there somewhere.

This should also get some tests to make sure it is covering those cases you mentioned and that we are taking the right paths.

[QuLogic]

Suggested change

	if ('edgecolor' not in kwargs) or (
	kwargs['edgecolor'] == 'face'):
	if kwargs.get('edgecolor', 'face') == 'face':

But why would 'face' mean patch edgecolor?

[smartlixx]

Because edgecolor = face means using facecolor value for edge. This suggests the user specifies facecolor, and would like to fill the LineString.

[greglucas]

I'm almost tempted to raise here. Why are we allowing a linestring to be filled at all?

[greglucas]

It has been a while since I looked at this, but it looks like it still needs tests.

[greglucas]

I'm almost tempted to raise here. Why are we allowing a linestring to be filled at all?

[smartlixx]

It has been a while since I looked at this, but it looks like it still needs tests.

Before I move on, I have to address the current test error, which I have no clue how to address. Any help would be appreciated.

================================== FAILURES ===================================
_________________ Test_Axes_add_geometries.test_styler_kwarg __________________
self = <cartopy.tests.mpl.test_axes.Test_Axes_add_geometries object at 0x0000005D80019910>
ShapelyFeature = <MagicMock name='ShapelyFeature' id='401595362560'>
add_feature_method = <MagicMock name='add_feature' id='401595271776'>
    @mock.patch('cartopy.mpl.geoaxes.GeoAxes.add_feature')
    @mock.patch('cartopy.feature.ShapelyFeature')
    def test_styler_kwarg(self, ShapelyFeature, add_feature_method):
        ax = GeoAxes(plt.figure(), [0, 0, 1, 1],
                     map_projection=ccrs.Robinson())
        ax.add_geometries(mock.sentinel.geometries, mock.sentinel.crs,
                          styler=mock.sentinel.styler, wibble='wobble')
    
        ShapelyFeature.assert_called_once_with(
            mock.sentinel.geometries, mock.sentinel.crs, wibble='wobble')
    
>       add_feature_method.assert_called_once_with(
            ShapelyFeature(), styler=mock.sentinel.styler)
C:\Miniconda3-x64\envs\test-environment\lib\site-packages\cartopy\tests\mpl\test_axes.py:108: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\Miniconda3-x64\envs\test-environment\lib\unittest\mock.py:919: in assert_called_once_with
    return self.assert_called_with(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <MagicMock name='add_feature' id='401595271776'>
args = (<MagicMock name='ShapelyFeature()' id='401595547712'>,)
kwargs = {'styler': sentinel.styler}
expected = call(<MagicMock name='ShapelyFeature()' id='401595547712'>, styler=sentinel.styler)
actual = call(<MagicMock name='ShapelyFeature()' id='401595547712'>, styler=<function GeoAxes.add_geometries.<locals>.styler at 0x0000005D80F8C790>)
_error_message = <function NonCallableMock.assert_called_with.<locals>._error_message at 0x0000005D80F7B790>
cause = None
    def assert_called_with(self, /, *args, **kwargs):
        """assert that the last call was made with the specified arguments.
    
        Raises an AssertionError if the args and keyword args passed in are
        different to the last call to the mock."""
        if self.call_args is None:
            expected = self._format_mock_call_signature(args, kwargs)
            actual = 'not called.'
            error_message = ('expected call not found.\nExpected: %s\nActual: %s'
                    % (expected, actual))
            raise AssertionError(error_message)
    
        def _error_message():
            msg = self._format_mock_failure_message(args, kwargs)
            return msg
        expected = self._call_matcher(_Call((args, kwargs), two=True))
        actual = self._call_matcher(self.call_args)
        if actual != expected:
            cause = expected if isinstance(expected, Exception) else None
>           raise AssertionError(_error_message()) from cause
E           AssertionError: expected call not found.
E           Expected: add_feature(<MagicMock name='ShapelyFeature()' id='401595547712'>, styler=sentinel.styler)
E           Actual: add_feature(<MagicMock name='ShapelyFeature()' id='401595547712'>, styler=<function GeoAxes.add_geometries.<locals>.styler at 0x0000005D80F8C790>)

[greglucas]

I think the problem is that the sentinel isn't callable, so then we go into your if clause and build up the dictionary instead. If you are able to make new tests that test all of the various paths, then you can probably replace this test, or you could potentially change it to a Mock() object that is callable...

>>> import unittest.mock
>>> callable(unittest.mock.sentinel)
False
>>> callable(unittest.mock.Mock())
True

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}