Improves upon previoius fixes to gophermap parsing #130

sloum · 2020-02-01T17:58:44Z

sloum commented

2020-02-01 17:58:44 +00:00

This should work better than the previous fix as it handles all cases. I believe what was breaking is that someone started a line with a tab and as a result line[0][0] was not possible. This update will make any line in which len(line[0]) < 1 make line[0] = "i", resulting in the line being treated as a comment. Which I think is good behavior for such a malformed line.

This should work better than the previous fix as it handles all cases. I believe what was breaking is that someone started a line with a tab and as a result `line[0][0]` was not possible. This update will make any line in which `len(line[0]) < 1` make `line[0] = "i"`, resulting in the line being treated as a comment. Which I think is good behavior for such a malformed line.

asdf was assigned by sloum

2020-02-01 17:58:50 +00:00

sloum added the

bug

gopher

labels 2020-02-01 17:59:08 +00:00

sloum referenced this pull request

2020-02-01 17:59:14 +00:00

Fixes crash rendering broken gopher maps #128

asdf commented

2020-02-04 02:26:44 +00:00

This looks good. I've run some tests on this and it works as expected without a panic.

You probably also first link is another weird line being rendered as a link of unknown type:

"     		        /  ________ 	70"

This is 5 spaces, two tabs, some more spaces and the ascii art, a tab, then the number 70. The "item type" is 5 spaces.

This seems like it could be an appropriate way to handle this line, as Lynx also does similar, but I don't think it allows you to try to browse that link. You probably have already thought this through so if it's ok then no worries.

This looks good. I've run some tests on this and it works as expected without a panic. You probably also first link is another weird line being rendered as a link of unknown type: ``` " / ________ 70" ``` This is 5 spaces, two tabs, some more spaces and the ascii art, a tab, then the number 70. The "item type" is 5 spaces. This seems like it could be an appropriate way to handle this line, as Lynx also does similar, but I don't think it allows you to try to browse that link. You probably have already thought this through so if it's ok then no worries.

sloum commented

2020-02-04 04:24:05 +00:00

I did check against what lynx was doing and also noticed that they still rendered it as a link. I think showing it as ??? should be sufficient. It isn't too doable to dig in and try to validate and verify every link. A user can always :check [link id] to see where the link goes. At which point they will see that it is mangled. It may still appear at first glance to be a Bombadillo issue rather than an issue with the underlying map :-/ Not sure if there is anything to be done about it though.

I _did_ check against what lynx was doing and also noticed that they still rendered it as a link. I think showing it as `???` should be sufficient. It isn't too doable to dig in and try to validate and verify every link. A user can always `:check [link id]` to see where the link goes. At which point they will see that it is mangled. It may still appear at first glance to be a Bombadillo issue rather than an issue with the underlying map :-/ Not sure if there is anything to be done about it though.

asdf commented

2020-02-04 07:28:15 +00:00

Sorry I just noticed I mistook how the item type is represented and interpreted. Yeah I think we are agreed.

👍 1

asdf approved these changes 2020-02-04 07:28:24 +00:00

sloum commented

2020-03-05 05:34:41 +00:00

Closing due to a shortcoming of tildegit: inability to switch the branch this is being merged into. A new PR will be opened referencing this one.

sloum closed this pull request

2020-03-05 05:34:42 +00:00