search

qgis / qgis4.0_api

Tracker for QGIS 4.0 API related issues and developer discussion
3 stars 1 forks source link

Remove visitor pattern from QgsExpression #64

Closed m-kuhn closed 7 years ago

m-kuhn commented 7 years ago

QgsExpression supports the visitor pattern but it is currently unused throughout our codebase (and I don't think used outside either). The major candidate for this would have been the expression compiler which is now implemented with a different approach.

I think we should remove it to keep the code a little slicker and in case we will need it for something it can easily be reintroduced.

@wonder-sk

NathanW2 commented 7 years ago

The main question is why it's not used when the pattern is good for that use case?

On Mon, 3 Oct 2016 4:56 pm Matthias Kuhn notifications@github.com wrote:

QgsExpression supports the visitor pattern but it is currently unused throughout our codebase (and I don't think used outside either). The major candidate for this would have been the expression compiler which is now implemented with a different approach.

I think we should remove it to keep the code a little slicker and in case we will need it for something it can easily be reintroduced.

@wonder-sk https://github.com/wonder-sk

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/qgis/qgis3.0_api/issues/64, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXS3ExI8xaDdUl2Fc049yBe_nEtz1YPks5qwKcRgaJpZM4KMSXb .

m-kuhn commented 7 years ago

Because it wasn't so good for that use case. I can't find the discussion but in the end I think the problem was you'd need an additional argument to the accept method for which the type cannot be defined. The current approach doesn't have this problem and I think we can say it has proven to be stable.

wonder-sk commented 7 years ago

I don't mind if it gets removed... It is easier to traverse the expression tree just with one recursive function rather than fiddling with visitor.

I get the impression the pattern makes more sense in different scenarios - 1) if the internal structure is opaque (not allowed) to access internals directory or 2) there are various traversing/filtering strategies, so visitor may be used to avoid code duplication.