Move `ArcanistSpellingDefaultData` into a configurable JSON file
ClosedPublic
Actions

Authored by joshuaspence on Jul 2 2014, 10:06 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rARC494d974005eb: Move `ArcanistSpellingDefaultData` into a configurable JSON file

Summary

Currently, the ArcanistSpellingLinter loads data from ArcanistSpellingDefaultData, with no way to configure the linter from an .arclint file. Instead we should define a format for a "dictionary" file, of which the ArvcanistSpellingLinter can load and of which the paths are easily configured through .arclint.

Test Plan

Updated the test case and ran arc unit.

NOTE: I have removed the LINT_SPELLING_PICKY and LINT_SPELLING_IMPORTANT constants and replaced them with LINT_SPELLING_FULL and LINT_SPELLING_PARTIAL. This was done because it simplifies the implementation considerably and makes customization of the ArcanistSpellingLinter simpler, but also because these constants were not widely used in the existing implementation.

Diff Detail

Repository

rARC Arcanist

Branch

spellinglinter

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/lint/linter/ArcanistSpellingLinter.php:103	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 1493
Build 1493: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 23525.Jul 2 2014, 10:06 PM

joshuaspence retitled this revision from to Move `ArcanistSpellingDefaultData` into a configurable JSON file.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 2 2014, 10:06 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B1493: Diff 23525.Jul 2 2014, 10:07 PM

epriestley accepted this revision.Jul 3 2014, 1:52 AM

epriestley edited edge metadata.

epriestley added inline comments.

src/lint/linter/ArcanistSpellingLinter.php
106	Consider defining this relative to `phutil_get_library_root('arcanist')` so the class doesn't mysteriously stop working if it is moved.

This revision is now accepted and ready to land.Jul 3 2014, 1:52 AM

Define dictionary relative to arcanist library root

Harbormaster completed remote builds in B1495: Diff 23532.Jul 3 2014, 2:03 AM

Rename "fullword" to "exact"

Harbormaster completed remote builds in B1496: Diff 23533.Jul 3 2014, 2:08 AM

joshuaspence added inline comments.Jul 3 2014, 3:22 AM

src/lint/linter/ArcanistSpellingLinter.php
103–108	Do you think that this is the right approach? The alternative would be to require that a spelling linter explicitly load all relevant dictionaries (which would break backwards compatibility). Another thought is that perhaps dictionaries should be able to be specified relative to the `resources/spelling` directory. i.e. if `"spelling.dictionaries": ["foo/bar.json"]` then a dictionary would be loaded the first existing path out of: `'arcanist/resources/spelling/foo/bar.json'` `$working_copy->getProjectRoot().'/foo/bar.json'`

joshuaspence added inline comments.Jul 3 2014, 3:34 AM

src/lint/linter/ArcanistSpellingLinter.php
76	Ironically, I appear to have misspelled "misspelling".

I think the given approach is reasonable. Copy/pasting the base data into your project or copy-pasting it to start your local file seem like mostly approaches (you aren't really going to miss that much by us adding 3 more words per year to the upstream) and they let you remove words. If there's a ton of demand for it or something we could add another config option later to include it explicitly, e.g.:

spelling.dictionaries = custom.json
spelling.include-default-dictionary = true

But I'd guess this won't come up for, like, at least a year.

mostly approaches

Er, mostly sensible approaches.

Yeah, I don't want to overcomplicate it right now (YAGNI). But I do want to add spelling.rules.exact and spelling.rules.partial configuration.

The spelling.include-default-dictionaries option is nice because it offers a more-sane way to provide a default without extending the .arclint stuff to support a default value.