Enable SLF001

[callumforrester]

          If this gets done there is a ruff rule (`SLF`) to enforce it.

Originally posted by @joeshannon in https://github.com/DiamondLightSource/blueapi/issues/360#issuecomment-2083163465

[callumforrester]

@coretl Suggestion: By default, private member access should be forbidden by CI

[coretl]

I agree with this for production code, but I think test code should be allowed to use private member access, otherwise it is often difficult to get coverage up enough.

How about we enable it, but then turn it off for all test files with https://stackoverflow.com/a/77680374?

I welcome a discussion on this: @DominicOram @GDYendell @gilesknap @evalott100 @callumforrester @DiamondJoseph @abbiemery @dperl-dls and anyone else who has an opinion

[callumforrester]

I'm increasingly against it even for test code following our blueapi experience. Tests should test the API of a class/module and treat it like a black box, so when you change the internal implementation you don't have to update the tests. We didn't do that with blueapi and now it's slowing down refactoring etc.

Note that in compiled (sensible) langauges this is simply non-optional and developers have to make it work. I think we're basically just arguing for the same here.

[DominicOram]

I agree with @coretl that I wouldn't want this forbidden in tests. Given we're coming from a codebase with basically no unit tests (GDA) I worry more about people writing code without tests than I do about them writing code that accesses private things. We should reduce barrier of entry on tests as much as possible.

I'm happy to have it in prod though. We can always use # noqa if we have a significant issue.

[callumforrester]

Hmm, good point RE: barrier to entry. Although I do think it has a much lower effect on the barrier to entry if it's an automated tool that produces clear error messages, which ruff generally does. I would hope combining that with coverage requirements means that people will generally write tests. It's the same model as black, a fast-feedback mechanism that pushes the code in a desirable direction on the scale of seconds rather than hours/days.

[DominicOram]

I think there are genuinely places where it's significantly harder to test without looking at private things and I suspect that the initial reaction that people will have when hit by this is to either drop the test or make the private variable public to get round it, both of which are worse than accessing the private variable IMO. I'm perfectly happy for it to be enforced on blueapi, you could probably even convince me to enforce it on dodal but I do not want to enforce it across the org. I also disagree with the comparison with black, which is mostly formatting, this feels more like forcing functional changes on people.

[GDYendell]

I don't hold a strong opinion on this, but my feeling is enforce it in tests and use noqa or maybe some tests subdirectories that are allowed and some not allowed. I would be keen to see some examples where it could be difficult.

It might be interesting for this implemented in blueapi to see what changes are required to make it compliant and whether we all think it is an improvement.

[callumforrester]

That's a good point, could "strongly encourage" it in tests but still have noqa as an option, so @DominicOram's newcomers still have an option other than dropping the test...

Would only need to make sure that's clear to them, adding a noqa is in vscode's suggested actions with the ruff plugin, would that be sufficient?

[joeshannon]

I would tend to agree with Gary and Callum however I'm coming from a predominantly Java background where the compiler prevents this - the public API of the class is tested and private methods are an implementation detail. Perhaps the landscape is more nuanced in python.

[coretl]

I suggest we try this on ophyd-async, and you try this on blueapi, then we make the decision after seeing just how many noqas we need to put it. I'm still erring on the side of turning it off in tests by default, people can always turn it back on if they want...

[dperl-dls]

Python explicitly makes everything accessible with a principle of "we're all adults here and we take responsibility if our code breaks because we accessed something marked with an underscore". I think it's fine to enforce not doing that as a style point in production code but I'm pretty against doing it for tests. It is often the best way to test things, and I think efforts to get around it are usually a lot worse. I also think it's bad to have "just put in a #noqa" as something we encourage people to do often, because they will start to just put them everywhere.

[DominicOram]

Adding an example I'm currently working on and why this would be painful in tests, particularly in dodal. I have a mock device with an internal signal that needs to be set when I set the device:

class MyDevice(Movable)
  def __init__(self, prefix: str, name: str = ""):
    self._internal_signal = epics_signal_rw(float, "BLAH")

  async def set(self, value)
    await self._internal_signal.set(MAGIC_VALUE)
    #  Do some other stuff

Whilst _internal_signal is private to MyDevice from the point of view of the codebase it is (in prod) pointing to an external PV and so it's part of the requirement of the class that the signal is set. I can test this like:

async def test():
   await my_device.set(0)
   assert_mock_put_called_with(my_device._internal_signal, MAGIC_VALUE)

I'm not sure how I would test this if I wasn't allowed to reference _internal_signal from my test without a significant amount of very messy patching of epics_signal_rw, which sort of defeats the point of https://github.com/bluesky/ophyd-async/issues/94

[callumforrester]

I admit this is somewhat down to personal taste, but I generally prefer to add a constructor arg for tests only (in this case so you can pass in MAGIC_VALUE) rather than do private access for tests. The reason being that I can refactor the internal logic without having to edit the tests, but the tests still check for regressions. This is also what I would have to do in most compiled languages.

[dperl-dls]

Sure, it's personal taste, but it will have effects on peoples' workflows. Putting testing features in your prod code means people will use them in their prod code and now you're stuck supporting people initialising your device with MAGIC_VALUE_2 as well. In my view that's quite a bit worse than the above solution. I don't think there's a whole lot of benefit in making the code look more like what people do when there are limitations to the language they're using, it's frankly horrible when people try to write java in python in other situations (see much of the GDA Jython code for examples...) and I don't think we should do it in this case either.

Anyway, how would that help you to check whether _internal_signal has been set?

[DominicOram]

I admit this is somewhat down to personal taste, but I generally prefer to add a constructor arg for tests only (in this case so you can pass in MAGIC_VALUE) rather than do private access for tests. The reason being that I can refactor the internal logic without having to edit the tests, but the tests still check for regressions. This is also what I would have to do in most compiled languages.

As @dperl-dls said, MAGIC_VALUE isn't the thing I'm worried about, it's _internal_signal. Are you suggesting that I should have to pass in an implementation for _internal_signal into the constructor?

[callumforrester]

No, you want to set the value of the internal signal at the beginning of the test, right? I was suggesting just passing the value in through the constructor. I was more generally suggesting that adding code that serves no other purposes than to make it easier to write tests is a lesser evil than private member access.

[stan-dot]

private member access tests internal logic, not the API of a class. if tests treat a piece of software as a black box and don't bother about the internals then they need to be changed less often. If you refactor internal logic then the same tests should still pass.

It is often the best way to test things, and I think efforts to get around it are usually a lot worse.

@dperl-dls private package member vs public setter is just syntactic sugar, that's the third way compared to #noqa

[DominicOram]

No, you want to set the value of the internal signal at the beginning of the test, right?

No, I'm not setting that in the test, I'm calling assert_mock_put_called_with on the private my_device._internal_signal and so asserting that the "internals" of the object do what I expect. My point is they aren't internals, they're calls to EPICS and so are part of the external API of the device. If your suggestion is that we pass in signals to every device with internal signals I think that's messy, has a higher barrier of entry and goes against the whole structure in ophyd_async of sim backends and connecting with mock=True.

Regardless, I think I've now convinced myself that I would disagree with putting it on dodal. Though I understand the arguments for slower changing more "core" libraries, where I think correctness is more important than velocity and ease of entry. In the interests of not piling more comments on a already quite long discussion, if people would like to keep pushing this beyond the "core" repos @coretl suggested can I request a quick in person discussion about it?

[callumforrester]

At this point I am bikeshedding so I'm happy to stop. I think enabling for src but not tests is a reasonable compromise. Will likely change for a number of core packages though.

[GDYendell]

Somewhat relevant. Pyright (and pylance) also check this with the reportPrivateUsage rule. This is in strict mode only, but can be individually switched on if the project doesn't want to enable full strict mode. It does apply this rule to tests when enabled.