Search for pubspecs surrounding analysis contexts #152
Conversation
|
To view this pull requests documentation preview, visit the following URL: Documentation is deployed and generated using docs.page. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
TimWhiting
changed the title
|
I figured I'd investigate some of the issues that users have been having getting custom_lint to work in their projects so that more people can adopt it and start using it with Riverpod. An analysis option file by itself seems to delimit the analysis context, but it isn't required to have a pubspec file alongside it. I believe searching the parent directories for the pubspec.yaml is the best solution for this particular issue. Let me know if you forsee any issues with this approach. |
| workspace.dir('package').createSync(recursive: true); | ||
| test( | ||
| 'throws MissingPubspecError if package does not contain a pubspec', | ||
| skip: true, //#148 |
There was a problem hiding this comment.
Not fond of skips. We should update the test to match instead
|
Overall I'm fine with the changes, but this needs testing. To begin with, I'm not sure when this happens exactly. I haven't tried to reproduce this, and I haven't had this issue. I do have analysis_options.yamls which are not associated with a pubspec.yaml myself. Riverpod is exactly in this situation. But custom_lint is working fine for me. |
| /// | ||
| /// This is a folder that contains both a `pubspec.yaml` and a `.dart_tool/package_config.json` file. | ||
| /// It is either alongside the analysis_options.yaml file, or in a parent directory. | ||
| Directory findProjectDirectory( |
There was a problem hiding this comment.
This handles looking for a project directory which contains at minimum a pubspec next to a .dart_tool/package_config.json (and in the case of hosted dependencies in the pubcache, the package config file is not necessary).
There was a problem hiding this comment.
Why do we care about searching for the existence of a package_config.json here? Shouldn't searching only for a pubspec be enough?
There was a problem hiding this comment.
I'm not sure why this was resolved. I still don't get why we are looking for package_config.jsons
To begin with, there's a high chance that if there's a missing package_config, then #168 should take care of it.
|
I've iterated on this and hopefully solved the test errors. Ready for another review. |
|
Note that the CI is passing on master. Could you maybe sync with master (while fixing conflicts)? |
|
Thanks for fixing the CI. Is this good to review for you? |
|
Yeah |
| ); | ||
| // Expect one context root for the workspace and one for the test folder | ||
| expect(customLintWorkspace.contextRoots.length, equals(2)); | ||
| }, |
There was a problem hiding this comment.
Could you remove those trailing commas and format again? Tests typically don't use them
|
Can someone write down what has been done, what is left, and what are the blockers? It would simplify the process for someone else to take this over. This is quite an annoying issue (no good workaround exists). I am considering picking this up, please explain the current status. |
|
@shilangyu There are still unresolved conversations and there are now conflicts. Both need to be fixed :) |
|
Sorry for not replying earlier. I'm no longer using flutter much in my day job, and don't really have the time to dedicate to this PR, I'm happy for someone else to take my work and finish it. |
|
The unresolved comments seem purely stylistic. Once rebased and corrected this will be good to merge or does it require extra architecture work? I am trying to gauge the level of codebase knowledge this requires to pick it up. |
|
@shilangyu There are some architectural comments. The ones about not throwing and not requiring a package_config.json |
|
I rebased this branch in my fork (https://github.com/shilangyu/dart_custom_lint/commits/main), but after looking at the other comments on this PR it seems like it requires deeper understanding of the feature to finish this PR. I do not want to by accident introduce regressions and I unfortunately do not have time to dive deeper into this topic. |
|
Closing as this is quite out of sync. |
Fixes #148