Auto-increment default names for PRs #334
Conversation
|
Thanks for the PR! Unfortunately there is a bug with your implementation: you always generate a new name for the experiment, but the new name should only be generated for the As for the n+1 queries, it shouldn't really be a problem (I don't expect to have more than 5 runs for a single PR). If you still want to avoid that you could select the count of experiments pointing to the same PR (checking the API URL in the database). Example of the bug
|
|
Ah, that makes sense. I'll take another shot at this tonight then. |
|
Okay, I think it should work now. If not, I'll try to catch you on Discord to discuss some more. Let me know if you want me to squash these commits or anything. |
|
There are still a few quirks in the implementation: if a second run is created and then edited, the edit command uses the first run's name. Example of the bug
The way I'd like to see this implemented is the following:
If you have any doubt just ping me on Discord! |
Also, don't auto-increment user-supplied names
| if name_supplied { | ||
| name | ||
| } else { | ||
| let new_name = auto_increment_experiment_name(&data.db, &name)?; |
There was a problem hiding this comment.
By passing as argument the previously stored name this is generating weird results, like last-name-1-1-1 (like pr-123-1-1-1-1).
| @@ -15,7 +15,17 @@ pub fn ping(data: &Data, issue: &Issue) -> Result<()> { | |||
| } | |||
|
|
|||
| pub fn run(host: &str, data: &Data, issue: &Issue, args: RunArgs) -> Result<()> { | |||
| let name = get_name(&data.db, issue, args.name)?; | |||
| let name = { | |||
| let name_supplied = args.name.is_some(); | |||
There was a problem hiding this comment.
This can be factored out as a standalone function, so it's easily testable.
See discussion at rust-lang#334
|
I think that commit addresses the issues you brought up and behaves as specified; let me know if I missed something. |
| name = format!("pr-{}-{}", issue.number, idx); | ||
| idx = idx | ||
| .checked_add(1) | ||
| .expect("too many similarly-named pull requests"); |
There was a problem hiding this comment.
Can you avoid panicking here? I'd prefer for the server not to crash if someone creates way too many experiments.
|
Thanks! @bors r+ |
|
📌 Commit b763389 has been approved by |
…etroalbini Auto-increment default names for PRs Fix #306 I thought that the easiest way to go about this was to adjust `default_experiment_name()`, and to just have it automatically try new names in case of a clash until it finds one that works. This makes the assumption that there won't be too too many experiments for a given PR, since experiment `N` will make N calls to the database, which is only really okay if N is small. Let me know if you'd like me to do something smarter here, or to adjust a different area of the code instead.
|
☀️ Test successful - status-appveyor, status-travis |
|
Changes deployed. |
Fix #306
I thought that the easiest way to go about this was to adjust
default_experiment_name(), and to just have it automatically try new names in case of a clash until it finds one that works. This makes the assumption that there won't be too too many experiments for a given PR, since experimentNwill make N calls to the database, which is only really okay if N is small. Let me know if you'd like me to do something smarter here, or to adjust a different area of the code instead.