Fixes T7977.
- Move Indexed Languages and See Symbols From config to Repository
- Make symbol search skip projects
This also makes the default languages to Everything instead of Nothing.
Details
Details
- Reviewers
joshuaspence epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T7977: Associate Symbols with Repositories directly (Instead of arc projects)
- Commits
- Restricted Diffusion Commit
rP898ce6bace59: Search Symbols by Repository, not Project
- Browse files, click symbols.
- Use quick search to find symbols
- Browse revision, click symbols
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I didn't really catch anything substantive either, this looks great to me.
| resources/sql/autopatches/20150504.symbolsproject.1.php | ||
|---|---|---|
| 16 | This will potentially update each repo several times, leaving the last value we process as the effective value. I think that's basically fine, but ideally we would probably append to $sources and $languages so that each repo gets the list of all sources and all languages as its final value. | |
| src/applications/diffusion/controller/DiffusionRepositorySymbolsController.php | ||
| 38 | Prefer phutil_utf8_strtolower(), which has less terrible behavior on some inputs. | |
| resources/sql/autopatches/20150504.symbolsproject.1.php | ||
|---|---|---|
| 27 | I'm running this again now, and this fails on "bad getter", which makes sense. Not sure why I thought it worked yesterday. | |
Comment Actions
@epriestley: @joshuaspence raised a good point - How about we eliminate the "indexed languages" completely?
It looks like any value other than all is very unlikely (Just for people who hate extra dotted underlines?)
Comment Actions
Generally looks good. Some minor inlines.
| resources/sql/autopatches/20150504.symbolsproject.1.php | ||
|---|---|---|
| 18–19 | I don't quite follow why you use this instead of $project. | |
| 33 | Prefer phutil_json_decode, which throws an exception if there is a failure. | |
| 45 | As above. | |
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
| 766 | Is nonempty necessary? | |
| 771 | Is empty($indexed_langs) necessary? | |
| src/applications/diffusion/controller/DiffusionBrowseFileController.php | ||
| 271 | I wonder if getSymbolSources() should just always include the "self" repository? This seems logical, but might be a bit too magical. | |
| 276 | Is the empty check necessary? | |
Comment Actions
Updates to migration schema
I'm a little hesitant to make changes to the migration script, because it's finicky to test. I've made a good effort last round ("Diff 3"), but I forgot how I did it, so this round might actually be worse.
I did test it again and it probably should be fine.
Plus, this version is all about using db fields directly, which will only work if projects hadn't actually changed it's structures since whenever was the last time the install upgraded (It looks very old schema, so it might be true).
| resources/sql/autopatches/20150504.symbolsproject.1.php | ||
|---|---|---|
| 4 | I can see that would make sense. | |
| 18–19 | Are you asking "instead of $projects" here? If so, it's because symbolIndexProjects is no longer pulled with that query. | |
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
| 766 | I wasn't sure if getSymbolSources can return null, and wanted to play it safe. | |
| 771 | empty here means "default", which I want to be "everything"... Or remove the field completely. | |
| src/applications/diffusion/controller/DiffusionBrowseFileController.php | ||
| 271 | ahh... I don't really like that. | |
| 276 | Same as above (default). | |
| src/applications/repository/storage/PhabricatorRepositoryTransaction.php | ||
|---|---|---|
| 408 | extra space after "to" :3 | |
Comment Actions
Couple of adjustments I caught poking at this locally:
- Hit this while editing existing Arcanist projects:
I just removed the symbol code in the if ($request->isFormPost()) body.
- Fixed the to trivial typo.
- Made a minor capitalization tweak for consistency ("symbols from" -> "Symbols From").
- I just removed the highlighting note since it was pointing to the wrong place anyway, and filed T8235 to do a pass on the docs here. Another thing that might be useful is telling users what the setting does.
| src/applications/diffusion/controller/DiffusionRepositorySymbolsController.php | ||
|---|---|---|
| 126–139 | I think this links to the wrong config setting? | |
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
|---|---|---|
| 768 | This could raise a warning about $langs not being an array. | |