The complete comment system revamp #424
Conversation
Merge in commits from lionleaf's master.
The new restrictions I added should greatly reduce the number of false-positive hashtag identifications, while obviously still keeping existing hashtags working. Also removed some redundant syntax, making the regex nicer, and making it easier to remove the "hashtags cannot start with a digit" rule if we ever decide to do that.
Makes sure there's no clickable pollutants in our code blocks
He's a tough guy, but he just needs us to clear the way for him a little bit, y'know?
Merge in commits from lionleaf's master.
Makes `make lint-python` pass successfully, where lines need to be <100 characters long.
After a discussion on Discord with @lionleaf, I am removing the 2-char minimum limit for hashtags. The problems that one-character hashtags have caused should be fixed anyway, and noone uses single-character hashtags anyway. So, as lionleaf seems to insist on having them, I am adding them back.
Enable hashtags beginning with numbers, since the space precedence check makes them safe to use now. Also make sure that those tags need to have at least one letter, to prevent senseless tags and make it more like twitter (I guess... we can always remove that very easily).
Merge in new commits from lionleaf's master
… magic links Also works with `http(s)://` and `www.` prefixes. Fixes issue lionleaf#173 (look at the last one or two comments).
General linting clean-ups. Don't know if the wrap_content function is supposed to have 6 spaces on the left, and 4 on the right, but it ain't broke, so I won't fix it.
Just cleaning up the files as I browse through all the .py files in the /tests folder. Made it have 2 newlines before each `def`, as the linter requires us to (?), and also cleaned up some spelling and grammar issues in the long multiline comment.
Do I sound smart now?
There was a problem hiding this comment.
Good progress.
Thanks a lot for adding new tests! That's super helpful to avoid future errors. I added some small comments.
A few more points:
- I think you might have issues with a comment like
This is some code: \color: #fff` `. If that works, I'd like to see a test. Can be done in a later pull request, since it's already slightly broken. - I wonder if we could enable number-only hashtags now? They caused problems with character-codes but I think that should be fixed by requiring spaces. Can be done in a separate PR
And I think I know what's causing you some issues:
-
There is another hashtag regex when the hashtags are added to the database. Take a look at
models.pyline 138. (This means I would have to do a migration to update all the old hashtags to match the new system, but I can deal with that later) -
You mentioned you have to refresh to see the new hashtags. This is likely caused by the ordering of
insert_magic_linksandinsert_code_blocksinget_urlized_text()from serializers.py line 43. This is how the code that makes links in the comment before you refresh the site.
If you fix 1. and 2. I think the tests will start passing.
| @@ -20,7 +21,7 @@ def test_insert_magic_links_replaces_user_with_valid_characters(self): | |||
| def test_insert_magic_links_bypasses_user_with_invalid_characters(self): | |||
| self.assertEqual( | |||
| 'u/a1$', | |||
| 'u/a1$' | |||
| insert_magic_links('u/a1$') | |||
There was a problem hiding this comment.
Yeah, but now a test fails... So I have to either switch it back, or introduce a new regex test to avoid inserting anchors into partially correct usernames. I have came up with such a regex, but users would have to insert a space after the username. So I thought about allowing dots and commas after the username as well, but I don't know if it's all too good to deal with this issue like that. I said this on discord as well, but noone replied :/
There was a problem hiding this comment.
I'm ok with u/a1$ becoming <a href='/u/a1'>u/a1</a>$ which I believe is how it works today?
You can change this test to insert_magic_links_does_not_linkify_invalid_username_characters or something similar and change it to the way it works now.
There was a problem hiding this comment.
Yeah, but it's kind of annoying, and doesn't feel right. If a username contains invalid characters, why match it? It's invalid!
I dunno... maybe I can find a better way of handling this.
There was a problem hiding this comment.
You can require a whitespace, end-of-line or punctuation at the end? Similar to with hashtags?
There was a problem hiding this comment.
Well, with hashtags it's whitespace or nothing, and that's before the hashtag :/... And then with endings there's the whole ordeal of what to define as punctuation, and what's just an invalid character. But as I said, I need to have a think about this.
|
Thanks for the info. I will certainly have a look at the models.py. The refreshing thing woud be qood to fix, but I think that I'll move that onto another PR. This is already quite a large one, and minor annoyances like this are not a major priority right now. I will also have a look at the code thing you sent, but your formatting kinda messed up there, so not sure exactly what that is supposed to say. As for the number-only hashtags, the requirement for a letter kinda put itself in place, and I just kept it there, as it seems to match the way twitter hashtags work, and y'know, twitter knows their stuff when it comes to hashtags 😉 Also, number-only hashtags probably wouldn't be very meaningful anyway. Also also, wht if I make a comment saying "My dweet is #1"? I don't want the "#1" to be a hashtag! Anyways, I have quite a lot of school work to do for the next few days, but hopefully I can have a look (and maybe finally get this PR ready) next friday/weekend :) Thanks again for the stuff! |
Also made naming of tests more consistent, and added some tests for http://, since the current autocrop regex handles those, and since it does, it's a nice thing to have. Say, for example, if someone makes a typo typing the long URL manually (new users perhaps?) or otherwise...
|
Okay, so I fixed the formatting in that code comment of yours, and pasted it into my localhost version of dwitter. The only thing that I could find that could possibly fail is the double backticks as in `whatever `a``, and we already test for that, so I'll just ignore that for now, unless you can clarify what issue you have found. So all I need to do now, is solve the usernames thing.... That was easy :D |
Punctuation allowed immediately after a user mention: " " "" "," ":" "." ";" "?" "!"
E501, W291
Honestly... too many to count.
|
Okay, so I fixed all of the stuff you mentioned above, and put tests in place for the new punctuation stuff. As of right now, all d/ links, u/ links, and hashtags have to be followed by a space, end-of-string, comma, colon, semi-colon, question mark, or exclamation point. Parentheses and quotes will be added in soon to fix 2-3 more tests.... However, I kinda need your help, as half of my tests are now failing due to 'unbalanaced parentheses' in the same file, at the same place, when I haven't even touched that file... 🤔 After we fix that, we're pretty much ready to merge :D |
|
The error is on this line: |
|
Looks like I messed up github markdown in an earlier comment. Let me try again; what happens with a comment like this (today #FFFFFF would become a clickable hashtag): Does it look ok? And I guess it might be silently added to hashtags, so even if #FFFFFF doesn't become clickable the dweet can now be found at h/FFFFFF ? |
|
Should fix #334 |
(Don't merge yet!)
I was initially going to create just a separate branch for the tests for the new hashtag system, but I ended up having all the new changes in this branch. Anyway, this pull request covers all the new functionalities and improvements for the comment system on Dwitter:
dwitter.net/d/123will automatically turn intod/123NOTE: The first two bulletpoints seem to require a refresh to work, after you post a comment using them... I'm not sure if this will be the case in production, but it's not a major issue anyway. Just thought I'd let you know, so that you would know. They should show up normally for other users viewing comments.
Overall, just minor things really, but they fix many of the bugs and annoying technical issues with the current comment system, and greatly increase the accuracy of the hashtag system.