Skip to content

Remove FreshRSS_Searchable for better types#5212

Merged
Alkarex merged 2 commits intoFreshRSS:edgefrom
Alkarex:simplify-searcheable
Mar 22, 2023
Merged

Remove FreshRSS_Searchable for better types#5212
Alkarex merged 2 commits intoFreshRSS:edgefrom
Alkarex:simplify-searcheable

Conversation

@Alkarex
Copy link
Member

@Alkarex Alkarex commented Mar 18, 2023

The interface was not used, and it was preventing more precise types for the different searchById() methods, as they each have different input and output types.

The interface was not used, and it was preventing more precise types for the different `searchById()` methods, as they each have different input and output types.
@Alkarex Alkarex added the System care Everything related to system care label Mar 18, 2023
@Alkarex Alkarex added this to the 1.22.0 milestone Mar 18, 2023
@ColonelMoutarde
Copy link
Contributor

Suggestion : The interface allows making sure that the method ::searchById($id) is well implemented, it has a real utility.
It allows among other things to make test like SomeClass instanceof FreshRSS_Searchable.
I think that this Pr risks to bring more verbose code later.
It is better to make evolve the interface

@Alkarex
Copy link
Member Author

Alkarex commented Mar 21, 2023

I would like to point you to the YAGNI (You aren't gonna need it) principle

it has a real utility

It is not used at all at the moment, so it does not make sure of anything, and therefore has no utility. Plus, it is not even the same types for all implementations, so it is not a good case for an interface.

more verbose code later

It is having this interface that makes the code more verbose right now.

Here is a bit of background https://betterprogramming.pub/avoiding-premature-software-abstractions-8ba2e990930a
(I had another article in mind, more focussed on this case, but cannot find it at the moment...)

@Alkarex
Copy link
Member Author

Alkarex commented Mar 22, 2023

Let's move on. It is easy to add again, should it ever become relevant, and in the meantime, it stands in the way

@Alkarex Alkarex merged commit 1a06165 into FreshRSS:edge Mar 22, 2023
@Alkarex Alkarex deleted the simplify-searcheable branch March 22, 2023 07:26
@ColonelMoutarde
Copy link
Contributor

ColonelMoutarde commented Mar 22, 2023

Let's move on. It is easy to add again, should it ever become relevant, and in the meantime, it stands in the way

You’r right ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

System care Everything related to system care

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants