[core] Language lifecycle#4060
Conversation
we need to remove usages of LanguageVersion::getLanguageVersionHandler
needs a dependency mechanism to prevent apex from being pruned
adangel
left a comment
There was a problem hiding this comment.
Thanks for the amazing work! 🎉
I've updated the task list on the PR description and added some doc tasks - I'll work on them and update this PR.
Could you have a look at the task "Missing unit tests for new language" whether this is done or still needed? I'm not sure what you mean by that.
Other than that, I don't think my other comments need to be addressed immediately (especially extracting LanguageHandler into own classes, which could create another bunch of changes, that are better done separately).
|
|
||
| // TODO replace those with actual language properties when the | ||
| // CLI syntax is implemented. | ||
| props.setProperty(LanguagePropertyBundle.SUPPRESS_MARKER, config.getSuppressMarker()); |
There was a problem hiding this comment.
Maybe that's worth to document: with the cli option for suppress marker, you configure it for all languages. With the env var for the property, you configure it for a specific language.
If both a present, one should take precedence. I think, the more specific one should override the general one - that means, if both CLI option for suppressmarker is given and the env for the language property, the env should be used.
There was a problem hiding this comment.
Ok, it seems the implementation is the other way around as I expected. Then it's all the more important to have the behavior documented to avoid surprises.
--suppress-marker overrides any configuration, that is provided by env vars.
| */ | ||
|
|
||
| package net.sourceforge.pmd.lang.apex.internal; | ||
| package net.sourceforge.pmd.lang.apex; |
There was a problem hiding this comment.
Maybe ApexLanguageProcessor should stay internal - at least, JavaLanguageProcessor is internal as well...
There was a problem hiding this comment.
Actually the language processors should probably be published at some point. I would like rules to be able to use them to initialize themselves exactly once, for instance, parse their property values into JClassTypes using the TypeSystem.
oowekyala
left a comment
There was a problem hiding this comment.
Thanks for the review Andreas!
the task "Missing unit tests for new language" is about adding unit tests for the new language base classes. I'll finish this on the weekend
…d7-language-lifecycle
adangel
left a comment
There was a problem hiding this comment.
Thanks! I'll merge this next 🎉
Describe the PR
(This contains #4049)
I'm opening this a bit early because I think it's a crucial step forward for the PMD 7 branch and I want to give it visibility. I will spend some more time adding tests and documentation, and improving the PR description.
This introduces the following binary incompatibilities:
Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)