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
Add article inspector mapping (part 4 of 4) #3870
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
+ added fetcher tests
tonisevener
added
Hold
Dependent PR
PR is dependent on another PR - merge dependent PR first and update branch before merging
labels
Feb 23, 2021
tonisevener
changed the title
Add article inspector mapping (part 4)
Add article inspector mapping (part 4 of 4)
Feb 23, 2021
benkaday3218
approved these changes
Mar 6, 2021
Closing this for now to clean up GitHub, will reopen once we get closer to heavy development on it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Dependent PR
PR is dependent on another PR - merge dependent PR first and update branch before merging
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes Phabricator ticket: https://phabricator.wikimedia.org/T270426
Do not review until #3869 is merged and this PR is updated with
main
.Notes
CombinedSentence
struct, as well as a way to generateNSAttributedString
s for eachCombinedSentence
.Test Steps
ArticleInspectorController
at the point thatself.combinedSections
is assigned.CombinedSentence
'sannotatedData
property (note I also added some light unit tests that we can flesh out more fully as we run into issues).I also added the attributed string generation method as a helper on
CombinedSentence
. I figure we will call this only after we have chosen a sentence (that will be done in a later Phabricator task). IMO there's no reason to generate attributed strings for every sentence if we're only going to need them for one. Because of this there's no way yet to test this method in the app - for now you can set a breakpoint in thetestAttributedString
unit test method to inspect those strings that are generated