Add newline to bookmark formatting + fix crashes when going up from root + make going up work in an index w/ a trailing slash #2

lel · 2019-09-28T06:09:09Z

lel commented

2019-09-28 06:09:09 +00:00

Bookmarks don't work at the moment, or at least more than one bookmark doesn't work at the moment. They all end up piled on one line, because gi.to_map_line(), which would appear to be AV-98's equivalent to VF-1's gopher_item_to_line(), doesn't do a newline at the end of its formatting. This fixes that.

lel changed title from ~~Add newline to string formatting~~ to Add newline to bookmark formatting + fix crashes when going up from root

2019-09-28 06:43:54 +00:00

lel changed title from ~~Add newline to bookmark formatting + fix crashes when going up from root~~ to Add newline to bookmark formatting + fix crashes when going up from root + make going up work in an index

2019-09-28 07:29:33 +00:00

lel commented

2019-09-28 07:32:41 +00:00

I found a couple crashes too (both when trying to go up from root but entirely unrelated in cause) so I guess those will also go in here.

The second commit changes your "remove trailing empty string" bit in GeminiItem.up from using a while loop to just an if (Edit: my final commit ends up removing this block entirely in favor of what I think is a better solution that fixes another bug). I don't know if there was some reason you used a while loop but I can't think of any situation where splitting the path in the fashion seen here would result in multiple empty strings in pathbits.

As for why it's necessary to change this while to an if, self.path is itself the empty string if you're actually at the very, very root, by which I mean something like just, say, gemini://carcosa.net . As a result, your split results in a list of two empty strings and you while away the first one and the second one and now suddenly you have the empty list and you crash with an IndexError when the next iteration tries to check if the last element is the empty string because there is no last element.

So because there doesn't appear to be any reason why you need to use a while over an if here, and using a while instead of an if causes a crash on sites like carcosa.net, etc, commit #2 changes this to an if.

As for commit #3, it makes GeminiItem.up actually return a GeminiItem when going up from root instead of just returning the string for the url because that means that when it's called by do_up, do_up calls _go_to_gi on a string instead of a GeminiItem, and then _go_to_gi immediately tries to check that string's scheme attribute, which of course it doesn't have, so it throws an AttributeError.

Commit #4, which I realized should probably be made as I was typing this comment up, changes the way I fix that, because at first I just wrapped your "self.url" return value in GeminiItem and instantiated a new GeminiItem because I wasn't exactly thinking, but actually what I should have done was just return self.

Commit #5, which I realized had to be made as I was typing that paragraph, makes the up function actually work in indexes with a trailing slash now that it's not crashing.

Okay, so the way you're splitting the path is by using os.path.split, which gives you a tuple of everything up to the last slash, and everything after the last slash. And then you're turning it into a list. And the way you're checking if you're in root is by seeing if there is only one element in that list.

So there will always be only one element in the list if you're in an index with a trailing slash, which means it will always think it's in root. Commit #5 fixes this by just using rstrip to remove a trailing slash if one exists before calling os.path.split, that way os.path.split is actually splitting to the directory above it. And since we're handling trailing slashes with rstrip, we no longer need to pop an empty string from the end of the list, because there will never be one.

I honestly think the rstrip solution is just cleaner and I don't see any unintended consequences coming from it (???)

I found a couple crashes too (both when trying to go up from root but entirely unrelated in cause) so I guess those will also go in here. The second commit changes your "remove trailing empty string" bit in GeminiItem.up from using a while loop to just an if (Edit: my final commit ends up removing this block entirely in favor of what I think is a better solution that fixes another bug). I don't know if there was some reason you used a while loop but I can't think of any situation where splitting the path in the fashion seen here would result in multiple empty strings in pathbits. As for why it's necessary to change this while to an if, self.path is itself the empty string if you're actually at the very, very root, by which I mean something like just, say, gemini://carcosa.net . As a result, your split results in a list of two empty strings and you while away the first one and the second one and now suddenly you have the empty list and you crash with an IndexError when the next iteration tries to check if the last element is the empty string because there is no last element. So because there doesn't appear to be any reason why you *need* to use a while over an if here, and using a while instead of an if causes a crash on sites like carcosa.net, etc, commit #2 changes this to an if. As for commit #3, it makes GeminiItem.up actually return a GeminiItem when going up from root instead of just returning the string for the url because that means that when it's called by do_up, do_up calls _go_to_gi on a string instead of a GeminiItem, and then _go_to_gi immediately tries to check that string's scheme attribute, which of course it doesn't have, so it throws an AttributeError. Commit #4, which I realized should probably be made as I was typing this comment up, changes the way I fix that, because at first I just wrapped your "self.url" return value in GeminiItem and instantiated a new GeminiItem because I wasn't exactly thinking, but actually what I should have done was just return self. Commit #5, which I realized *had* to be made as I was typing that paragraph, makes the up function actually work in indexes with a trailing slash now that it's not crashing. Okay, so the way you're splitting the path is by using os.path.split, which gives you a tuple of everything up to the last slash, and everything after the last slash. And then you're turning it into a list. And the way you're checking if you're in root is by seeing if there is only one element in that list. So there will always be only one element in the list if you're in an index with a trailing slash, which means it will always think it's in root. Commit #5 fixes this by just using rstrip to remove a trailing slash if one exists before calling os.path.split, that way os.path.split is actually splitting to the directory above it. And since we're handling trailing slashes with rstrip, we no longer need to pop an empty string from the end of the list, because there will never be one. I honestly think the rstrip solution is just cleaner and I don't see any unintended consequences coming from it (???)

lel changed title from ~~Add newline to bookmark formatting + fix crashes when going up from root + make going up work in an index~~ to Add newline to bookmark formatting + fix crashes when going up from root + make going up work in an index w/ a trailing slash

2019-09-28 07:57:04 +00:00

solderpunk closed this pull request

2019-10-06 14:36:08 +00:00

solderpunk commented

2019-10-06 14:41:34 +00:00

Thanks for this! Sorry for the slightly slow reaction.

Sign in to join this conversation.

No reviewers