-
Notifications
You must be signed in to change notification settings - Fork 149
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
Use a manual parser for rust-timer queue
#1994
base: master
Are you sure you want to change the base?
Conversation
Instead of a regex. This allows the user to pass the arguments in an arbitrary order.
d852d3a
to
c7d14cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
Here's a free test shuffling the arg order.
#[test]
fn queue_command_parameter_order() {
insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=3 exclude=c,a include=b"),
@r###"Some(Ok(QueueCommand { include: Some("b"), exclude: Some("c,a"), runs: Some(3) }))"###);
}
@@ -57,3 +57,6 @@ jemalloc-ctl = "0.5" | |||
serde = { workspace = true, features = ["derive"] } | |||
serde_json = { workspace = true } | |||
toml = "0.7" | |||
|
|||
[dev-dependencies] | |||
insta = "1.40.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want/need insta? Managing the snapshots files can be cumbersome, and it's yet another tool to use and learn. It's fine if we want to make good use of it in the future, and not a fancy assert_eq!(format!("{obj:?}"), "Struct { field: 'bla'} ")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I'm refactoring this, I want to do it properly. Note that I'm not using snapshot files, I'm using the inline snapshots, which makes this much easier to grok, I think.
I'm already anticipating future changes (like your PR to add the backends
) parameter. It's no fun updating tens of tests anytime you change the structure of the thing that you parse. Snapshot testing makes this much easier.
I will document in readme what to do to update the snapshots though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Insta adds snapshot files locally when a test fails, the pending snap file. I saw that when adding the test I shared. It didn’t remove it when the test passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, these things. Right, I guess we can add them to .gitignore
, but they should be removed after you use cargo insta review
I think (the snapshots shouldn't really be modified manually).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have insta
installed to do a cargo insta review
anyways. We're testing a struct with 3 fields, we don't really need all this test infra imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already 23 tests in the file, I ain't gonna rewrite all the asserts to make sure we parse exactly what we think we do, every time we change the parsing code 😆 insta
was already super helpful for me when developing this PR.
site/src/request_handlers/github.rs
Outdated
.map(|index| line[index + prefix.len()..].trim()) | ||
})?; | ||
|
||
let args = bot_line.strip_prefix("queue").map(|l| l.trim())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GH never reformats comments, right? That is, it will not introduce unexpected \n
s that would separate the command from some of its arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh, that would be quite surprising I think. GH actually has a bunch of formats of the comments, like text, HTML, MD etc., but I think (hope) that here we work with the raw text. We were already requiring =
to be right after include
etc. before, so I think that this should be fine.
format!("Error occurred while parsing comment: {error}") | ||
} | ||
}; | ||
main_client.post_comment(issue.number, msg).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to take special care (like sanitization) in posting the error message to GH as its text will be partially controlled by users' input commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting idea. I don't think so, it should have the same rules as when you create the comment manually. I guess that the user could make the bot print "Unknown command argument SEND MONEY TO THE FOLLOWING BITCOIN ADDRESS TO SPONSOR THE RUST FOUNDATION", but I don't think that they can XSS or something like that (that would be really bad hole in GH's security).
When we do basically this for the |
I wanted to also refactor the |
Yeah, I've only used include a small number of times in the past. I was mostly thinking of fixing the multiple queue commands bug rather than making my life easier to parse |
Decided to go all the way and reimplemented also the |
Instead of a regex. This allows the user to pass the arguments in an arbitrary order.