internal_link: Parse topic permalinks #729
Conversation
sm-sayedi
force-pushed
the
issue-684-parse-topic-permalinks
branch
from
3c4996c to
a72eb7b
Compare
lib/model/internal_link.dart
Outdated
| // cannot use lowerCamelCase `with` as it is a reserved keyword in Dart | ||
| // ignore: constant_identifier_names | ||
| With, |
There was a problem hiding this comment.
Fun.
Let's call it with_. The leading capital reads to me like it's a different kind of thing — like not an enum value at all, but a class or something like a class.
There was a problem hiding this comment.
It will generate the same "with_" string value in the enum map which is not compatible with the API! 🙂
There was a problem hiding this comment.
Ah, that can be adjusted, though. The JSON serialization is all nicely customizable with more annotations — for a value of an enum, it's @JsonValue:
https://pub.dev/documentation/json_annotation/4.9.0/json_annotation/JsonValue-class.html
There was a problem hiding this comment.
Done! Thank you.
Also, as this is my first time using the json_annotation package, I have a hard time working with it:
When I run the dart run build_runner watch --delete-conflicting-outputs command, all of the other auto-generated files are also modified, such as lib/api/model/events.g.dart, lib/api/route/account.g.dart, and more — and some are deleted such as lib/host/android_notifications.g.dart. So far, I have used git restore file to reject changes to those files, but I am not sure if I am doing things correctly from the start. Also, the files that these modified files are used in, gives an error like this:
Target of URI hasn't been generated: './android_notifications.g.dart'.
Try running the generator that will generate the file referenced by the URI.dart[uri_has_not_been_generated]
Even though I run the previous command, and also do a flutter clean and flutter pub get, those errors won't go away!
There was a problem hiding this comment.
Hmm, curious. That's not a symptom I've seen. Please start a chat thread in #mobile-team and let's debug there.
There was a problem hiding this comment.
(For cross-reference: this turned out to be a newlines issue, of CRLF vs. LF.)
sm-sayedi
force-pushed
the
issue-684-parse-topic-permalinks
branch
from
a72eb7b to
f28998c
Compare
There was a problem hiding this comment.
Looks and works great!
Moving on to mentor review, cc @hackerkid.
rajveermalviya
added
mentor review
There was a problem hiding this comment.
Thanks for working on this @sm-sayedi. Changes looks good to me :)
hackerkid
added
maintainer review
There was a problem hiding this comment.
Thanks, @sm-sayedi!
One nit in the commit message, otherwise LGTM.
internal_link: Parse topic permalinks
These links (`/with/NNNN`) are just accepted and ignored in this PR,
they will be interpreted in #683.
Fixes: #684
The links themselves aren't accepted and ignored; it's only the with operator that's accepted and ignored. So for example (from the added test cases), /#narrow/pm-with/1,2-group/with/2 is interpreted as a DM narrow with users 1 and 2, just like /#narrow/pm-with/1,2-group would be.
I noticed some pre-existing disorganization and duplication in this file; I might try to clean things up as a followup once this is merged.
|
Thanks @sm-sayedi and reviewers! This looks good, apart from the point Chris made above about the commit message. I'll fix that up and merge, so I can include this in the upcoming v0.0.16 release to get us all ready for starting to use this new API feature on the server. |
gnprice
force-pushed
the
issue-684-parse-topic-permalinks
branch
from
f28998c to
ebb41b6
Compare
These links (
/with/NNNN) are just accepted and ignored in this PR, they will be interpreted in #683.Fixes: #684