intellij-community
intellij-community copied to clipboard
IDEA-290939 support more default values in code generation
Writing test case... do not merge this until additional commit! https://youtrack.jetbrains.com/issue/IDEA-290939
Ready to get review @akozlova. Review points:
- Defaulting for
Optional
perfectly reasonable, then also good for Collection<A>, List<A>, even String? - At this code, will find the default by
PsiType#getCanonicalText
. If not found, find again usingPsiType#equalsToText
. Is there a case that needequalsToText
instead ofgetCanonicalText
? if not, theequalsToText
method need to be removed.
Thank you for your effort in advance.
I don't understand when you would use these defaults actually. To me, in mostly 99% of cases these defaults would be overridden and it's better to have nothing than smth which pretend to be ok. Could you please elaborate why you need this? While the code is ok (see my comments). Thanks
@akozlova The reason is same with Optional
, as it basically a single size of List
.
The use case is, if we need kind of quick test to run a code without runtime error and redundant null check:
interface PersonRepository {
List<Person> getAllPersons();
}
class SQLPersonRepository implements PersonRepository { }
class EmptyPersonRepository implements PersonRepository {
// Auto gen with defaults
{
PersonRepository repo = getRandomRepositoryWithReflection();
for (Person person : repo.getAllPersons()) {
// at this case, commonly, we're not expecting the `getAllPersons()` will returns null.
}
Edit --
The reflection
and quick test
I'm not to meaning as it is a edge case. If we need such empty pattern like the EmptyPersonRepository
above, and the functions in PersonRepository
many in there, replacing nulls manually to empty will be annoying.
Generally, it's making defaults more complicated. Optional was implemented because it's a known bad practice to return null from Optional method, there is no such problem with other types (at least other types were not invented to prevent nulls). Thus I generally don't like the change, I am sorry.
One more argument against: if you insert Collections.emptyList()
, you most probably insert import. If people would change the code (and most probably they would), then import would be unused and probably would be forgotten in the code (e.g. if optimize imports on commit is disabled). This way, these defaults would make the code worser. At the same time, I don't see big advantages - if you run test and fix the code - it's ok to fix NPEs as well as empty collections.
Again, I am sorry for complaining in the PR but it would land for all users.
I think, it's possible to implement via template, do you want me to help you with it?
@akozlova I agree with the import issue, even we can use optimize import, in many place it can being leaved as unused. Just remind about the argues,
The Optional implemented because nulling it is a known bad practice
- Then why is it a bad practice essentially? because
Optional
invented to replacenull
. - Then why
Optional
invented? as Oracle says, avoid NPE and redundant null checking.
In this context, even if only Optional
invented to avoid null
, it's goal is actually to avoid NPE and redundant null checking.
Then why we not treat List.empty()
to avoid NPE and redundant checking? it's essentially a same. Reminding again:
List<Person> persons = personRepo.getAllPersons();
if (persons != null) {
for (Person person : persons) {
// TODO
}
}
Insert Collections.emptyList() causes unused imports
Even it seems trivial, I 100% agree it is worse. Then we can List.of()
on Java9+?
Please close the PR and issue still it not motivating to go.