Pull request #61 from kallithea-incoming#default
Pull Request Content
13 comments (0 inline, 13 general) First comment
Showing 3 commits
3 | patrickdp | f65144842edc |
9 years ago
|
Draft
default
|
||
2 | patrickdp | 4e53d2a4f00d |
9 years ago
|
|||
1 | patrickdp | c2d5d4fbc79f |
9 years ago
|
Draft
default
|
Common ancestor: 591effa1fc4d
41 files changed with 151 insertions and 97 deletions:
13 comments (0 inline, 13 general)
First comment
Thomas De Schampheleire (patrickdp)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
Status change: Under review
Mads Kiilerich (kiilerix)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
Thanks. I pushed the 2 changesets that didn't change since first iteration.
I will wait for a closer look, second opinion, and testing on the last changeset.
I like that the url method now lives so closely together with the routes definition it relies on instead of being in util like everything else. The extra import makes it explicit that routing depends on pylons. But also, in the existing code, that dependency is indirectly, by environment.py inserting make_map result in config['routes.map']. I guess people could have an opinion on that. Like if it would be better if the url dependency on pylons somehow was indirectly too.
I will wait for a closer look, second opinion, and testing on the last changeset.
I like that the url method now lives so closely together with the routes definition it relies on instead of being in util like everything else. The extra import makes it explicit that routing depends on pylons. But also, in the existing code, that dependency is indirectly, by environment.py inserting make_map result in config['routes.map']. I guess people could have an opinion on that. Like if it would be better if the url dependency on pylons somehow was indirectly too.
Thomas De Schampheleire (patrickdp)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
The pylons dependency in UrlGenerator is only for pylons.request.environ['routes.url']. It is not _really_ a dependency on pylons, but rather pylons happens to be the 'messenger' that we use to obtain a handle to the environment.
If we can get to the correct 'routes.url' via another path, I guess it could work just as fine.
For example, is 'pylons.request.environ['routes.url']' actually different than 'routes.url' ?
If we can get to the correct 'routes.url' via another path, I guess it could work just as fine.
For example, is 'pylons.request.environ['routes.url']' actually different than 'routes.url' ?
Mads Kiilerich (kiilerix)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
Yes, all this to get to the right Mapper instance created by make_map ...
I don't think there is a 'routes.url' - it is set in the environment by RoutesMiddleware as a UrlGenerator instance based on the 'routes.map' config set in load_environment.
I wonder how this would fit together in a clean TG2 app ...
I don't think there is a 'routes.url' - it is set in the environment by RoutesMiddleware as a UrlGenerator instance based on the 'routes.map' config set in load_environment.
I wonder how this would fit together in a clean TG2 app ...
Thomas De Schampheleire (patrickdp)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
I don't think a clean tg2 app uses Routes.
Thomas De Schampheleire (patrickdp)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
I don't think a clean tg2 app uses Routes.
Mads Kiilerich (kiilerix)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
Even without routes, there might be a pattern we can learn from.
Søren Løvborg (kwi)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
Once make_map is called (during startup/config phase), the route map doesn't change again. It seems to me that it would thus be cleanest to simply store the route map in a module-level variable, and keep it as an internal implementation details of the kallithea.config.routing module.
Code speaks louder than words, so see db1f0a6adcae for the gist of it.
Code speaks louder than words, so see db1f0a6adcae for the gist of it.
Søren Løvborg (kwi)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
Søren Løvborg (kwi)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
In general, I think it's fine to store static config data in global variables, as long as there's a well-defined "barrier" between time-of-definition and time-of-use. In the case of url, there's no reasonable use-case for generating URLs in the startup/config phase, so it's straightforward.
(On the other hand, kallithea.CONFIG and the associated problems are caused by the lack of a well-defined barrier.)
(On the other hand, kallithea.CONFIG and the associated problems are caused by the lack of a well-defined barrier.)
Thomas De Schampheleire (patrickdp)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
As I commented on the commit, the implementation does not look right to me.
Note by the way that in my rcextensions package, I add a new route to the mapper. I would prefer that to continue working, of course.
Note by the way that in my rcextensions package, I add a new route to the mapper. I would prefer that to continue working, of course.
Søren Løvborg (kwi)
8 years and 8 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
Status change: Approved
Yes, disregard my comment. (I still think my approach is the *right* one, except for the pesky detail that it admittedly doesn't actually *work*.) ;-)
Let's stick to this, then.
Let's stick to this, then.
Thomas De Schampheleire (patrickdp)
8 years and 7 months ago
comment
on pull request
"Turbogears2 migration: preparatory commits (v2)"
¶
These have landed...
Status change: