tahoe-lafs/tahoe-lafs
tahoe-lafs
/
tahoe-lafs
2
0
Fork 0
Code Issues Pull Requests 23 Packages Projects Releases 1 Wiki Activity

3263 port web directory.manifest.2 #665

Merged
meejah merged 10 commits from ticket3252-port-web-directory.manifest.2 into master 2019-12-21 23:50:47 +00:00
Conversation 3 Commits 10 Files Changed 5 +216 -66
meejah commented 2019-11-05 08:12:37 +00:00 (Migrated from github.com)
Copy Link

see https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3263#ticket

Ports the "manifest" part of allmydata.web.directory to twisted.web.template from nevow

This change is Reviewable

see https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3263#ticket Ports the "manifest" part of `allmydata.web.directory` to `twisted.web.template` from nevow <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/tahoe-lafs/tahoe-lafs/665) <!-- Reviewable:end -->
codecov-io commented 2019-11-05 10:05:49 +00:00 (Migrated from github.com)
Copy Link

Codecov Report

❗ No coverage uploaded for pull request base (master@c336467). Click here to learn what that means.
The diff coverage is 78.18%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #665   +/-   ##
=========================================
  Coverage          ?   89.76%           
=========================================
  Files             ?      166           
  Lines             ?    29203           
  Branches          ?     4144           
=========================================
  Hits              ?    26215           
  Misses            ?     2256           
  Partials          ?      732
Impacted Files Coverage Δ
src/allmydata/web/directory.py 92.56% <78.18%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c336467...67bd9c0. Read the comment docs.

# [Codecov](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665?src=pr&el=h1) Report > :exclamation: No coverage uploaded for pull request base (`master@c336467`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit). > The diff coverage is `78.18%`. [![Impacted file tree graph](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665/graphs/tree.svg?width=650&token=Ztmu9sr4vi&height=150&src=pr)](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665?src=pr&el=tree) ```diff @@ Coverage Diff @@ ## master #665 +/- ## ========================================= Coverage ? 89.76% ========================================= Files ? 166 Lines ? 29203 Branches ? 4144 ========================================= Hits ? 26215 Misses ? 2256 Partials ? 732 ``` | [Impacted Files](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665?src=pr&el=tree) | Coverage Δ | | |---|---|---| | [src/allmydata/web/directory.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS93ZWIvZGlyZWN0b3J5LnB5) | `92.56% <78.18%> (ø)` | | ------ [Continue to review full report at Codecov](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665?src=pr&el=footer). Last update [c336467...67bd9c0](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/665?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
exarkun (Migrated from github.com) requested changes 2019-11-08 20:05:27 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

Thanks. Comments left inline. In addition to reading the code, I spun up a node and tried out the manifest functionality. I wasn't able to exercise the "still running" branches easily but the other branches look good to me.

Thanks. Comments left inline. In addition to reading the code, I spun up a node and tried out the manifest functionality. I wasn't able to exercise the "still running" branches easily but the other branches look good to me.
src/allmydata/test/no_network.py
exarkun (Migrated from github.com) commented 2019-11-08 19:29:17 +00:00
Copy Link

Oof. That was unexpected. Can you file a ticket for replacing NoNetworkGrid(...) with an async-friendly API?

Oof. That was unexpected. Can you file a ticket for replacing `NoNetworkGrid(...)` with an async-friendly API?
src/allmydata/web/directory.py
exarkun (Migrated from github.com) commented 2019-11-08 19:33:18 +00:00
Copy Link

Bytes or unicode?

Bytes or unicode?
exarkun (Migrated from github.com) commented 2019-11-08 19:33:24 +00:00
Copy Link

Bytes or unicode?

Bytes or unicode?
exarkun (Migrated from github.com) commented 2019-11-08 19:35:43 +00:00
Copy Link

Looks like it returns an empty string (bytes or unicode?) sometimes. I suspect its contract with its caller is that it returns a "flattenable" which is any of a bunch of common types or an object adaptable to IRenderable. bytes and str are both in that bunch of common types.

Looks like it returns an empty string (bytes or unicode?) sometimes. I suspect its contract with its caller is that it returns a "flattenable" which is any of a bunch of common types or an object adaptable to `IRenderable`. `bytes` and `str` are both in that bunch of common types.
exarkun (Migrated from github.com) commented 2019-11-08 19:43:22 +00:00
Copy Link

Element itself is new-style. Hopefully the extra object here is redundant?

`Element` itself is new-style. Hopefully the extra `object` here is redundant?
exarkun (Migrated from github.com) commented 2019-11-08 19:46:38 +00:00
Copy Link

timdelta instead of int might be nice for this. int is more like the old code it should eventually supersede but timedelta makes the units self-documenting...

`timdelta` instead of `int` might be nice for this. `int` is more like the old code it should eventually supersede but `timedelta` makes the units self-documenting...
exarkun (Migrated from github.com) commented 2019-11-08 19:47:06 +00:00
Copy Link

For all of the string literals in this method and for the str call - bytes or unicode?

For all of the string literals in this method and for the `str` call - bytes or unicode?
exarkun (Migrated from github.com) commented 2019-11-08 19:47:52 +00:00
Copy Link

Pretty much just going to take that one on faith ... I see it is the same as the old code, anyway.

Pretty much just going to take that one on faith ... I see it is the same as the old code, anyway.
src/allmydata/web/directory.py
@ -948,0 +1043,4 @@
if cap:
root_url = URL.from_text(u"{}".format(root))
cap_obj = from_string(cap)
if isinstance(cap_obj, (CHKFileURI, WriteableSSKFileURI, ReadonlySSKFileURI)):
exarkun (Migrated from github.com) commented 2019-11-08 19:42:51 +00:00
Copy Link

What is that monitor attribute?

What is that monitor attribute?
meejah (Migrated from github.com) reviewed 2019-11-09 00:30:15 +00:00
src/allmydata/test/no_network.py
meejah (Migrated from github.com) commented 2019-11-09 00:30:15 +00:00
Copy Link

Seems there was one already: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2951

Seems there was one already: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2951
meejah (Migrated from github.com) reviewed 2019-11-09 10:30:31 +00:00
src/allmydata/web/directory.py
meejah (Migrated from github.com) commented 2019-11-09 10:30:31 +00:00
Copy Link

I guess this should also probably use hyperlink or similar .. and a general question here, because currently the Request objects will be Nevow ones (well, actually, Tahoe overrides that with MyRequest but ..) "until everything is ported", so do I just wrap everything in six.text_type() .. or is a better pattern to use e.g. u"{}".format(req.prepath[-1]) so that we don't have a bunch of six` stuff to get rid of later?

I guess this should also probably use `hyperlink` or similar .. and a general question here, because currently the `Request` objects will be Nevow ones (well, actually, Tahoe overrides that with `MyRequest` but ..) "until everything is ported", so do I just wrap everything in `six.text_type()` .. or is a better pattern to use e.g. `u"{}".format(req.prepath[-1]) so that we don't have a bunch of `six` stuff to get rid of later?
meejah (Migrated from github.com) reviewed 2019-11-17 02:18:07 +00:00
src/allmydata/web/directory.py
meejah (Migrated from github.com) commented 2019-11-17 02:18:07 +00:00
Copy Link

I'm going to go with the latter ..

I'm going to go with the latter ..
meejah (Migrated from github.com) reviewed 2019-11-17 02:33:01 +00:00
src/allmydata/web/directory.py
@ -948,0 +1043,4 @@
if cap:
root_url = URL.from_text(u"{}".format(root))
cap_obj = from_string(cap)
if isinstance(cap_obj, (CHKFileURI, WriteableSSKFileURI, ReadonlySSKFileURI)):
meejah (Migrated from github.com) commented 2019-11-17 02:33:01 +00:00
Copy Link

So ... I think it's supposed to always be an IMonitor (which I've documented, now) but also I think sometimes it's an object that just acts like that :/ ...that part, I'm not going to touch.

So ... I *think* it's supposed to always be an `IMonitor` (which I've documented, now) but also I think sometimes it's an object that just *acts* like that :/ ...that part, I'm not going to touch.
meejah (Migrated from github.com) reviewed 2019-11-17 02:35:21 +00:00
src/allmydata/test/no_network.py
meejah (Migrated from github.com) commented 2019-11-17 02:35:20 +00:00
Copy Link

I feel like aborting this PR because of the above is .. overkill? What I put in here was I guess just pragmatic: failing silently on e.g. a syntax error is bad, so this is the minimum to spit out something if grid-setup fails.

I feel like aborting this PR because of the above is .. overkill? What I put in here was I guess just pragmatic: failing silently on e.g. a syntax error is bad, so this is the minimum to spit out something if grid-setup fails.
meejah (Migrated from github.com) reviewed 2019-11-17 02:35:36 +00:00
src/allmydata/web/directory.py
meejah (Migrated from github.com) commented 2019-11-17 02:35:35 +00:00
Copy Link

👍

:+1:
meejah (Migrated from github.com) reviewed 2019-11-17 03:53:28 +00:00
src/allmydata/test/no_network.py
meejah (Migrated from github.com) commented 2019-11-17 03:53:28 +00:00
Copy Link

(but, i could take it out and do a separate PR .. that should be ultimately fixed by 2951 above?)

(but, i could take it out and do a separate PR .. that should be ultimately fixed by 2951 above?)
meejah commented 2019-11-19 01:58:52 +00:00 (Migrated from github.com)
Copy Link

I just merged master into this (to get rid of the conflict). (I did this instead of rebasing so comments are hopefully better preserved etc by github)

I just merged master into this (to get rid of the conflict). (I did this instead of rebasing so comments are hopefully better preserved etc by github)
exarkun (Migrated from github.com) reviewed 2019-11-19 14:00:33 +00:00
src/allmydata/test/no_network.py
exarkun (Migrated from github.com) commented 2019-11-19 14:00:33 +00:00
Copy Link

I feel like aborting this PR because of the above is .. overkill?

I agree. Let's not do that.

> I feel like aborting this PR because of the above is .. overkill? I agree. Let's not do that.
meejah commented 2019-12-18 00:17:25 +00:00 (Migrated from github.com)
Copy Link

bump. @exarkun when you have time it would be good to confirm the changes cover your comments. (p.s. the "still running" branches can be seen easily at least for directory-level operations .. for a single file it has to be fairly big)

bump. @exarkun when you have time it would be good to confirm the changes cover your comments. (p.s. the "still running" branches can be seen easily at least for directory-level operations .. for a single file it has to be fairly big)
exarkun (Migrated from github.com) approved these changes 2019-12-18 15:32:18 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

Thanks. I left a couple minor comments inline but this generally looks good to me. I did notice there is also some missing test coverage ... it was missing before though, too. What's the general thinking here? Should that test coverage be added here or should we make a follow-up ticket and address it later?

Overall, I approve of these changes and wouldn't mind seeing them merged. Address the review feedback as you see fit and then merge. Thanks again.

Thanks. I left a couple minor comments inline but this generally looks good to me. I did notice there is also some missing test coverage ... it was missing before though, too. What's the general thinking here? Should that test coverage be added here or should we make a follow-up ticket and address it later? Overall, I approve of these changes and wouldn't mind seeing them merged. Address the review feedback as you see fit and then merge. Thanks again.
src/allmydata/web/directory.py
exarkun (Migrated from github.com) commented 2019-12-18 15:30:17 +00:00
Copy Link

maybe allmydata.uri.from_string(...)? Then there's a bunch of is_xyz methods and some interfaces that can be checked.

maybe `allmydata.uri.from_string(...)`? Then there's a bunch of `is_xyz` methods and some interfaces that can be checked.
src/allmydata/web/directory.py
exarkun (Migrated from github.com) commented 2019-12-18 15:28:17 +00:00
Copy Link

There is a render_HTML above which I suppose is now obsolete? I don't see how it could be doing anything, given this new render_HTML method, anyway.

There is a `render_HTML` above which I suppose is now obsolete? I don't see how it could be doing anything, given this new `render_HTML` method, anyway.
meejah (Migrated from github.com) reviewed 2019-12-21 07:45:09 +00:00
src/allmydata/web/directory.py
meejah (Migrated from github.com) commented 2019-12-21 07:45:09 +00:00
Copy Link

I think what I meant by that comment was wanting e.g. is_chk(u"blarg") or so .. but yeah, I could use .from_string and make it an isinstance() instead of a startswith, which is probably better. Thanks :)

I *think* what I meant by that comment was wanting e.g. `is_chk(u"blarg")` or so .. but yeah, I could use `.from_string` and make it an `isinstance()` instead of a `startswith`, which is probably better. Thanks :)
Sign in to join this conversation.
Reviewers
No reviewers
exarkun
Labels
Clear labels   
Benchmarking and Performance

   
HTTP Storage Protocol

   
Nevow Removal

   
Python 3 Porting

   
not-for-merge
No Label
Benchmarking and Performance
HTTP Storage Protocol
Nevow Removal
Python 3 Porting
not-for-merge
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/tahoe-lafs#665
No description provided.
Delete Branch "ticket3252-port-web-directory.manifest.2"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?