cctbx / dxtbx

Diffraction Experiment Toolbox
BSD 3-Clause "New" or "Revised" License
2 stars 18 forks source link

Optional arguments being used as positional arguments in tests #195

Open graeme-winter opened 4 years ago

graeme-winter commented 4 years ago

e.g. in

    @staticmethod
    def simple(
        sensor,
        distance,
        beam_centre,
        fast_direction,
        slow_direction,
        pixel_size,
        image_size,
        trusted_range=(0.0, 0.0),
        mask=[],
        px_mm=None,
        mu=0.0,
        gain=None,
        pedestal=None,
        identifier="",
    ):

convention dictates that all arguments after image_size should be labelled in the calling routine - they are not in some cases (e.g. in tests) where they are assumed to follow in the correct order. This makes the API fragile.

Anthchirp commented 4 years ago

https://www.python.org/dev/peps/pep-3102/ allows

def function(
  arg1,
  arg2,
  *,
  keyword_argument_only=True,
  no_positional_arguments_allowed_here=True,
)

a possible alternative would be to pass a named tuple instead of up to 14 arguments.

graeme-winter commented 4 years ago

👆 strikes me as a very good idea. The initial positional arguments stay since they are somewhat sine qua non however any optional arguments should be added with parameter names attached, I like this and it will make it much quicker to debug.

Says introduced in 3.0, was it backported?

graeme-winter commented 4 years ago

Not 100% obvious how to make this play nice with boost::python exported classes though e.g. Panel 🤔

Anthchirp commented 4 years ago

Not backported to my knowledge. If it were backported it would be exceedingly hacky. This would be a breaking change, and therefore the Python 2.7 deprecation ticket would apply.

Anthchirp commented 4 years ago

For boost::python the answer is approximately no, which is not entirely surprising given that boost::python hasn't seen much active development for a long time. The straightforward solution would be to wrap the object within Python.