Add logic for duping radar #75
Conversation
|
Fixes #14 |
keith
force-pushed
the
ks/dup-radar
branch
6 times, most recently
from
61685eb
to
34a5242
Compare
|
Wouldn't it make sense to try to parse the data from open radar and put it in the right fields (as in #76), and only if this fails fall back to this behavior here? |
|
Any particular reason you think it's worth separating the text? In the end the display ends up the same (at least from our perspective, I'm not sure about internal to Apple employees) |
keith
force-pushed
the
ks/dup-radar
branch
2 times, most recently
from
31a2459
to
462acc3
Compare
|
It makes editing harder if it is not parsed since you have one relatively small field with all the info. Also, what happens if I add text to another field? Then I have this paragraph two times in the resulting radar? Also it simply looks nicer. Opening a radar and seeing everything in this one field in brisk looks like a bug at first. |
|
(Also I think the parser doesn't need to be rock solid. There is a fallback. So the parser can be improved over time to be able to parse more radars. Looking through open radar there seem to be a few different formats that people use.) |
| self?.showError(title: "No OpenRadar found", | ||
| message: "Couldn't find an OpenRadar with ID #\(id)") | ||
| return | ||
| } |
There was a problem hiding this comment.
I got this error when little snitch blocked the request, think it's worth separating that case?
| } | ||
| } | ||
|
|
||
| public func radarID(from string: String) -> String? { |
There was a problem hiding this comment.
No top-level type that makes sense to scope this under?
There was a problem hiding this comment.
It could be on a string extension if we wanted, not strong opinion.
There was a problem hiding this comment.
Nah was hoping there was a Radar struct or something
|
|
||
| for component in components { | ||
| if component.characters.last == ":", | ||
| let setter = sectionToSetter[String(component.characters.dropLast()).lowercased()] |
There was a problem hiding this comment.
Would guard this instead
BriskTests/OpenRadarTests.swift
Outdated
| @@ -1,7 +1,12 @@ | |||
| // swiftlint:disable line_length | |||
This adds a new window for duping a radar. It takes the ID of a radar, searches OpenRadar for the details about it, and then fills the normal UI with the values from it. The complex part of this is what we put into what fields in the UI from OpenRadar. The OpenRadar API just returns one big blog of summary data (exactly how radarweb displays it), so instead of parsing that, we're just filling the description with that. It ends up looking a little weird since we're only filling 1 of the 5 text views, but it shouldn't be a big deal. Also OpenRadar doesn't vend the `area` field sent to radar, so to default to something we just choose "other".
|
@michaelochs I ended up parsing the common case here, and falling back to putting it all in the first text view in the failure case. If you have a chance please try this out on master and let me know what you think! |
|
Most of the radars seem to work nice. A couple I tried run into the error: e.g. rdar://32937801, rdar://32934061 – but they use a slightly different format, so that's okay I guess! Awesome feature! (And great app btw.) |
|
Thanks 😄 So the first one there, I think I'm fine with not parsing. Even if we were a little more accepting, we still wouldn't get the The second case I think we should fix. I've seen quite a few radars that have an opening paragraph, but no heading for it, and then everything else is in the right format. |
This adds a new window for duping a radar. It takes the ID of a radar,
searches OpenRadar for the details about it, and then fills the normal
UI with the values from it.
The complex part of this is what we put into what fields in the UI from
OpenRadar. The OpenRadar API just returns one big blog of summary data
(exactly how radarweb displays it), so instead of parsing that, we're
just filling the description with that.
It ends up looking a little weird since we're only filling 1 of the 5
text views, but it shouldn't be a big deal. Also OpenRadar doesn't vend
the
areafield sent to radar, so to default to something we justchoose "other".