Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make use of modern-style quotes #197

Merged
merged 3 commits into from
Oct 17, 2016
Merged

Conversation

Lertsenem
Copy link
Contributor

(See issue #134)

Twitter introduced a new way of quoting, by simply putting the tweet url
as a quote. The web interface (and most clients) will then display a
preview of the quoted tweet, leaving more characters available for your
quote.

This patch change the default quote format from the old mode ("#comment
RT #owner #text") to the new one, introducing a new #tid keyword holding
the tweet id (needed to find the tweet URL). It's still possible to go
back to the old mode, by changing the QUOTE_FORMAT config parameter
back to "#comment RT @#owner #text".

(See issue orakaro#134)

Twitter introduced a new way of quoting, by simply putting the tweet url
as a quote. The web interface (and most clients) will then display a
preview of the quoted tweet, leaving more characters available for your
quote.

This patch change the default quote format from the old mode ("#comment
RT #owner #text") to the new one, introducing a new #tid keyword holding
the tweet id (needed to find the tweet URL). It's still possible to go
back to the old mode, by changing the QUOTE_FORMAT config parameter
back to "#comment RT @#owner #text".
Copy link
Owner

@orakaro orakaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. If you can get rid of str function then this can be merged

screen_name = str( tweet['user']['screen_name'] )
text = str( tweet['text'] )
tid = str( tweet['id'] )

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str function is not compatible with Python 2 unicode type, in case text is non-English character (ex: Japanese).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry about that. The str() functions for screen_name and text are not really needed, since these values are already printable as is. For tid however, I should keep the str() since tweet['id'] is an integer.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -24,7 +24,7 @@
// 'conversation': max tweet in a thread
"CONVERSATION_MAX" : 30,
// 'quote' format
"QUOTE_FORMAT" : "#comment RT #owner: #tweet",
"QUOTE_FORMAT" : "#comment https:\/\/twitter.com\/#owner\/status\/#tid",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note: even if we change value here it will not affect already installed user. I preserved their customized config file every update time. This can only help new users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but already installed users will still be able to change their configuration if they want to use this feature (like I did).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

To avoid messing up unicode non-latin characters.

This was a requested change.
During the config parse, comments are removed using a complex regexp,
matching (among other things) '//'. A problem arises when your
configuration contains a link of some sort (such as 'http://' ...). The
regexp then consider the '//' and what follows as a comment (and your
configuration is consequently considered corrupt).

Escapinf the '/' works for a time, but it fails as soon as you use the
'config' command at runtime, because the configuration is then
rewritten.

Therefore I changed the regexp used to match comments. It's a fairly
complex regexp though, and I'm not 100% positive my change is perfect.
Fell free to review it.
@Lertsenem
Copy link
Contributor Author

I did the change you requested in dd4ac7c.

I also corrected a small bug: after using the 'config' command, there was an error when parsing my configuration. The escaped string 'http://...' was rewritten unescaped as 'http://...' and the '//...' part was wrongly parsed as a comment. I put all details in 6bb36e2 commit message.

Copy link
Owner

@orakaro orakaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Actually i excluded QUOTE_FORMAT (along with 4 other config key) before showing user in the console in https://github.com/DTVD/rainbowstream/blob/master/rainbowstream/config.py#L48 so people are unlikely to use config command to set QUOTE_FORMAT as described.

Although fixing the regex has the downside that we no longer can read comment inside of any line, I agree that it helps setting format easier.

This will be merged in this weekend.

@orakaro orakaro merged commit 7ce368d into orakaro:master Oct 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants