Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

super + include #172

Open
stdweird opened this issue Oct 4, 2017 · 7 comments
Open

super + include #172

stdweird opened this issue Oct 4, 2017 · 7 comments

Comments

@stdweird
Copy link
Member

stdweird commented Oct 4, 2017

i ran into this a few times already, and wondered if the following would make sense:

given includes and/or loadpath with order dir1,dir2, when we have a template X in dir2, and do an include X and the template from dir2/X gets included.

but sometimes we want to customise (who doesn't), and would like to do the following: make a template dir1/X, that also includes dir2/X. we now rename dir1/X to dir1/XY and do an include X in dir1/X to get the default/normal behaviour.

keeping the name X is just much nicer; but it would require a super include X to include the dir2/X instead of creating a loop.
i use super because it looks to me this is very similar to what class inheritance uses.

maybe there's a good reason why this is not available in pan, but i can't think of one...

(i'm aware that aquilon injects a lot of pre and post templates automagically, so less of an issue there, but we aren't using aquilon yet 😄

@stdweird stdweird added this to the 10.4 milestone Oct 4, 2017
@stdweird
Copy link
Member Author

stdweird commented Oct 4, 2017

@ned21 @jrha @gombasg thoughts?

@gombasg
Copy link
Contributor

gombasg commented Oct 4, 2017

My first reaction is that this would be awfully difficult to follow, because despite the name, it would not be obvious at all which template would super include X do actually include.

Trying to pretend this has anything to do with inheritance would be a rather bad idea. Pan does not have any concept of objects, and this super construct would not behave like one would naively expect from any object-oriented language. Calling it include next X would be better, but it would still be difficult to follow.

If you want to make dir2/X customizable, then just go ahead and add a line like:

include if_exists("X_custom");

That would make the control flow much easier to follow. The reason we don't need this construct is not due to the templates Aquilon generates, but because we're not afraid of just editing dir2/X directly 😄.

Note that dependency tracking around LOADPATH is, let's say, sub-optimal at the moment. Given the example above, the dependency file originally will only have X PAN dir2 recorded. So if dir1/X suddenly appears, object profiles will not be recompiled, because the compiler has no idea that anything would want to use dir1/X.

@gombasg
Copy link
Contributor

gombasg commented Oct 4, 2017

Also, if you want to pretend there's some kind of inheritance going on, then I'd really expect dir1/X to be able to override e.g. functions defined with the same name in dir2/X, maybe even final variables - and then we're going down a rather slippery slope... Some restrictions like requiring both dir1/X and dir2/X to be declaration templates, otherwise making include next X to fail, could help. But I'm still not convinced that this complexity would worth all the trouble. Really, you should improve dir2/X instead of hacking around it 😄

@stdweird
Copy link
Member Author

stdweird commented Oct 4, 2017

@gombasg thanks a lot for the feedback. i admit i didn't give it that much thought yet 😉

the syntax should actually be include next; there's nothing to specify wrt template name.

wrt include if_exists("X_custom");, i really do not like this to litter code. we have a lot of templates that might be modified. it's true that it makes the flow very explicit and easy to follow, but what do you add in X_custom? include if_exists('X_custom_custom')? i'd actually consider include if_exists("X_custom"); hacking instead of improving (but that's personal taste i guess).

i really like the idea that aquilon generates this for you, so it's there to use, but you don't have to add it yourself.
maybe each include Y should actually do a include custom/pre/Y;include Y; include custom/post/Y;? (it would slow down the compiler, but proper filesystem lookup cache could help a lot; btw is that not an issue in aquilon? or is the number of generated include if_exist statements significantly smaller than the total number of included templates).

i do not think super should start changing final varaibles or functions, that's too much, so i guess next is good enough / better.

your remark wrt LOADPATH (and includes) should be an issue in pan to add this to the documentation. i never realised it, but obviously quite dangerous.

@ned21
Copy link
Contributor

ned21 commented Oct 26, 2017

Discussed at RAL workshop. @gombasg will write up his proposed solution in this issue.

@jrha jrha modified the milestones: 10.4, 10.5 Jan 15, 2018
@jrha jrha modified the milestones: 10.6, 10.7 Apr 13, 2018
@stdweird
Copy link
Member Author

@gombasg any updates?

@jouvin
Copy link
Contributor

jouvin commented Apr 25, 2018

I'm just looking at the issue/proposal but I find it a very complicated and potentially ambiguous approach to something that can be done with a proper layout of templates. Waiting for @gombasg summary!

@jrha jrha removed this from the 10.8 milestone Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants