#3128. Initial commit #3129
#3128. Initial commit #3129
Conversation
|
(Just FYI: I'm working on this. ;-) |
There was a problem hiding this comment.
Many good things here! I'm not quite sure about the status—some parts of the code look like it is not nearly finished. So we need to find a good approach to check in this code such that it is well-understood what the status is, and what the code is expected to be able to do. We should probably talk about these things in an online meeting because it's a new situation.
| main(List<String> args) { | ||
| Config config = Config.fromJson(readConfig()); | ||
| Spec spec = Spec.fromTxt(config.specPath); | ||
| Co19 co19 = Co19(config.co19Dir); |
There was a problem hiding this comment.
Co19 goes along with Json, it's almost a word in this context. ;-)
tools/spec_coverage/bin/main.dart
Outdated
| findSpecChapters(co19.language.subDirs, spec.chapters); | ||
| } | ||
|
|
||
| void findSpecChapters(List<TestDir> testDirs, List<Chapter> chapters) { |
There was a problem hiding this comment.
I think this name is confusing. findSpecChapters may describe what's going on, but it makes no sense to find something and then not deliver the entities that were found, or doing something to them, or with them.
Perhaps the code is not doing anything (other than print) because it's unfinished? In that case we might want to wait a bit until we have some code that is sufficiently complete to be understood and used in some concrete cases.
It would still make sense to land various pieces of code that are reasonably well-understood (e.g., the handling of configurations in a *.json file will probably not need to change much, but findSpecChapters looks like it will change substantially before we can run those scenarios).
There was a problem hiding this comment.
Renamed to compareChaptersAndDirs
tools/spec_coverage/bin/main.dart
Outdated
| for (TestDir td in testDirs) { | ||
| bool found = false; | ||
| for(Chapter ch in chapters) { | ||
| if (td.name.toLowerCase() == ch.co19DirName.toLowerCase()) { |
There was a problem hiding this comment.
This is approximately O(n^2) where n is the number of chapters-or-dirs. That's probably not too bad (we have about 208 sections/subsections/subsubsections, and there are about 505 co19 directories), but it still seems reasonable to sort each of those lists and take the toLowerCase once and for all, and then proceed to search for matching names by holding an index into each of those two lists. That would be more like linear time.
There was a problem hiding this comment.
Thank you. Fixed! Please review.
| } | ||
|
|
||
| static String resolvePath(String relativePath) { | ||
| String basePath = Directory.current.path; |
There was a problem hiding this comment.
So this utility must be executed from the root of the package? OK, I think that's a rather common restriction (but it might be nice to have an error message if the current directory at startup doesn't look like a package root).
tools/spec_coverage/lib/spec.dart
Outdated
| class Spec { | ||
| late final List<Chapter> chapters; | ||
|
|
||
| Spec.fromTxt(String path) { |
There was a problem hiding this comment.
Perhaps this could be a static method returning Spec (possibly renamed as Specification). The class could then have a private constructor accepting the list, and chapters wouldn't have to be late.
There was a problem hiding this comment.
Found another way. Fixed.
| @@ -0,0 +1,61 @@ | |||
| import 'spec.dart'; | |||
|
|
|||
| class SpecParser { | |||
There was a problem hiding this comment.
OK, so this class depends in detail on the format of spec.txt. It's a good thing that this dependency is encapsulated to this class alone.
There was a problem hiding this comment.
Thank you for the very detailed and useful review! Updated, PTAL. For now the only that this tool does is comparing spec chapters and test directories names and print the output like:
Found spec for co19\Language\Generics
Not found spec for co19\Language\Generics\Superbounded_types. Chapters are:
instantiation_to_bound
super_bounded_types
variance
And we immediately realyse why the appropriate chapter was not found for the directory.
The next step is to parse spec text and find appropriate assertions there.
PTAL.
tools/spec_coverage/lib/spec.dart
Outdated
| class Spec { | ||
| late final List<Chapter> chapters; | ||
|
|
||
| Spec.fromTxt(String path) { |
There was a problem hiding this comment.
Found another way. Fixed.
tools/spec_coverage/bin/main.dart
Outdated
| for (TestDir td in testDirs) { | ||
| bool found = false; | ||
| for(Chapter ch in chapters) { | ||
| if (td.name.toLowerCase() == ch.co19DirName.toLowerCase()) { |
There was a problem hiding this comment.
Thank you. Fixed! Please review.
tools/spec_coverage/bin/main.dart
Outdated
| findSpecChapters(co19.language.subDirs, spec.chapters); | ||
| } | ||
|
|
||
| void findSpecChapters(List<TestDir> testDirs, List<Chapter> chapters) { |
There was a problem hiding this comment.
Renamed to compareChaptersAndDirs
Please note
Tools/spec_coverage/resources/readme.txt