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

New plugin: tangorin #3

Merged
merged 11 commits into from
Oct 12, 2016

Conversation

winterthediplomat
Copy link
Contributor

This is a (possible) implementation for the tangorin plugin that is already present in Descartes but not in Gauss.

I didn't write lots of Rust until now, so if you spot something that is not idiomatic, or could be done better, or just mistakes I overlooked, I'll be more than happy to fix them.

Fixes #2

impl Tangorin {
fn grep_kanji(&self, msg: &str) -> Option<String> {
match RE.captures(msg) {
Some(captures) => {
Copy link
Owner

@RoxasShadow RoxasShadow Oct 8, 2016

Choose a reason for hiding this comment

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

Some(captures) => captures.at(1).map(|e| e.to_owned())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


fn retrieve_from_selector(&self, doc: &kuchiki::NodeRef, selector: &str) -> Option<String> {
if let Some(match_) = doc.select(selector).unwrap().next() {
let node = match_.as_node().first_child().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

check the indentation here, it looks quite weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ssh on spotty connections is suffering and makes me miss this kind of errors. fixed.

if let Some(match_) = doc.select(selector).unwrap().next() {
let node = match_.as_node().first_child().unwrap();
let borrowed_text = node.as_text().unwrap().borrow();
let mut retrieved_text = borrowed_text.clone();
Copy link
Owner

Choose a reason for hiding this comment

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

I think this doesn't need to be mutable as you should be able to do everything in a single line, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now this not mutable anymore. Originally, I had some problems with borrowed_text lifetimes: to fix it I put each change to its own line, but didn't recompose things back.


Some(retrieved_text)
}
else{
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like much this line. What about an early return on the very top of the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored the whole thing, so early returns are not necessary anymore.

if let Some(match_) = doc.select("span[class=eng]").unwrap().next() {
let node = match_.as_node();
let children = node.children();
let mut meaning = "".to_string();
Copy link
Owner

Choose a reason for hiding this comment

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

String::new() in place of "".to_string()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

};

if let Ok(doc) = kuchiki::parse_html().from_http(&url) {
let romaji = match self.retrieve_from_selector(&doc, "rt"){
Copy link
Owner

Choose a reason for hiding this comment

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

What if we make a macro to reduce this redundant lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented a simple macro called get_text!. it definitely cuts the noise down.

let info : String = match self.retrieve_info(&doc) {
Some(retrieved) => {
let sanitized = retrieved.replace("\u{2014}", "").replace(".", "").to_lowercase();
(" (".to_string()+sanitized.as_str()+")").to_string()
Copy link
Owner

Choose a reason for hiding this comment

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

format!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i felt there was a better way to do that, but didn't look at the docs. fixed.

let sanitized = retrieved.replace("\u{2014}", "").replace(".", "").to_lowercase();
(" (".to_string()+sanitized.as_str()+")").to_string()
},
None => "".to_string()
Copy link
Owner

Choose a reason for hiding this comment

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

String::new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

None => "".to_string()
};

return server.send_privmsg(target, &format!("[Tangorin] {} ({} - {}): {}{}", &*kanji, &*kana, &*romaji, &*meaning, &*info))
Copy link
Owner

Choose a reason for hiding this comment

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

I'm pretty sure that filling a proper struct with these data and using the ToString trait would work great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really think so. Creating an useless struct just to use .to_string is not as readable as a format!, especially considering that the retrieved information is not shared with other plugins or stored in any way.

Copy link
Owner

Choose a reason for hiding this comment

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

At least let's try to format that string properly (splitting the vars in multiple lines just work fine) as right now that's. struct as very lightweight btw :)

assert_eq!("PRIVMSG test :[Tangorin] 頑な (かたくな - katakuna): obstinate (usually written using kana alone)\r\n",
&*get_server_value(&server));
}

Copy link
Owner

@RoxasShadow RoxasShadow Oct 8, 2016

Choose a reason for hiding this comment

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

useless line here, one is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed

Some(match_) => {
let node = match_.as_node().first_child().unwrap();
let borrowed_text = node.as_text().unwrap().borrow();
Some(borrowed_text.clone().trim().to_string())
Copy link
Owner

Choose a reason for hiding this comment

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

to_owned() is better than clone() as it is an its specialisation

static ref RE: Regex = Regex::new(r"!tangorin (\S+)").unwrap();
}

macro_rules! get_text {
Copy link
Owner

@RoxasShadow RoxasShadow Oct 9, 2016

Choose a reason for hiding this comment

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

try_option to be more similar to try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


fn retrieve_meaning(&self, doc: &kuchiki::NodeRef) -> Option<String> {
match doc.select("span[class=eng]").unwrap().next() {
None => None,
Copy link
Owner

@RoxasShadow RoxasShadow Oct 9, 2016

Choose a reason for hiding this comment

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

Can you think a way to avoid this None => None in many places? like an early return or so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed privately, I've switched to Options.map when the conversion is easy and meaningful.

if let Some(text) = child.as_text() {
meaning.push_str(text.borrow().as_str());
}
else{
Copy link
Owner

Choose a reason for hiding this comment

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

put a space after else please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

let following_siblings = node.following_siblings();

match following_siblings.select("i[class=d-info]") {
Err(()) => None,
Copy link
Owner

Choose a reason for hiding this comment

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

Err(_)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


match following_siblings.select("i[class=d-info]") {
Err(()) => None,
Ok(mut info_sibs) => {
Copy link
Owner

@RoxasShadow RoxasShadow Oct 9, 2016

Choose a reason for hiding this comment

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

you can avoid {...} as using direct => match is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

match following_siblings.select("i[class=d-info]") {
Err(()) => None,
Ok(mut info_sibs) => {
match info_sibs.next() {
Copy link
Owner

Choose a reason for hiding this comment

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

info_sibs.next().map(|n| ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

match root.first_child(){
None => None,
Some(match_) => {
match match_.as_text() {
Copy link
Owner

Choose a reason for hiding this comment

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

match_.as_text().map(|n| ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -30,36 +30,30 @@ impl Tangorin {
}

fn retrieve_from_selector(&self, doc: &kuchiki::NodeRef, selector: &str) -> Option<String> {
match doc.select(selector).unwrap().next() {
Some(match_) => {
doc.select(selector).unwrap().next().map(|match_| {
let node = match_.as_node().first_child().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

there's an indentation problem here maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

if let Some(text) = child.as_text() {
meaning.push_str(text.borrow().as_str());
}
else {
Copy link
Owner

Choose a reason for hiding this comment

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

elsif?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


fn retrieve_from_selector(&self, doc: &kuchiki::NodeRef, selector: &str) -> Option<String> {
doc.select(selector).unwrap().next().map(|match_| {
let node = match_.as_node().first_child().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

indentation problem here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgot to fix it in the previous patch :V

}

fn inner_text(&self, root: &kuchiki::NodeRef) -> Option<String> {
match root.first_child(){
Copy link
Owner

Choose a reason for hiding this comment

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

put a space before the {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

let kana = try_option!(self.retrieve_from_selector(&doc, "rb"));
let kanji = try_option!(self.retrieve_from_selector(&doc, "span[class=writing]"));
let meaning = try_option!(self.retrieve_meaning(&doc));
let info : String = match self.retrieve_info(&doc) {
Copy link
Owner

Choose a reason for hiding this comment

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

don't put any space before the : (i.e. let info: String =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops :///:

let kanji = try_option!(self.retrieve_from_selector(&doc, "span[class=writing]"));
let meaning = try_option!(self.retrieve_meaning(&doc));
let info : String = match self.retrieve_info(&doc) {
Some(retrieved) => format!(" ({})", retrieved.replace("\u{2014}", "").replace(".", "").to_lowercase()),
Copy link
Owner

Choose a reason for hiding this comment

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

WDYT about String::remove here?

Copy link
Contributor Author

@winterthediplomat winterthediplomat Oct 12, 2016

Choose a reason for hiding this comment

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

unfortunately String::remove does not do what we thought it does:
Removes a char from this String at a byte position and returns it..
String::replace is still the only method to do that.

@RoxasShadow
Copy link
Owner

This is cool, just a couple of request changes and we can merge this – thanks a lot 👍

@RoxasShadow RoxasShadow merged commit 363645f into RoxasShadow:master Oct 12, 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.

Port Descartes' tangorin module to Gauss
2 participants