Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed get*List() to return an empty list on null paths, rather than null #455

Closed
wants to merge 1 commit into from

Conversation

krinsdeath
Copy link
Contributor

Not sure why this was changed from the previous Configuration class, it was a fantastic feature of the class and I am moderately annoyed that I have to catch it myself.

Anyway, shouldn't be too intrusive so there you are.

@chrisyco
Copy link

I think the idea was any changes you made to the returned list would be saved along with the rest of the config. If you return a new ArrayList and a plugin adds to it, that ArrayList won't get saved, will it?

Plus, I would much rather have my plugin crash when I make a typo in the node name than have it silently created. I'd suggest changing the name to something like getOrCreate...List()

EDIT: screw everything I just said

@krinsdeath
Copy link
Contributor Author

It's already building a new ArrayList() anyway. This just allows things like for (String str : section.getStringList("path")) { } to not throw NPEs. None of the methods return a direct instance of the list in the configuration section, so I'm not sure what your point is.

As for the plugin crashing thing, why? The way it is now, you need to get an instance of the list and either check that it's null (you can't even check isEmpty() because it'll NPE) or surround it in try/catch just to make sure your plugin is stable. If you do use a lot of lists, this can be very cumbersome and annoying if you want it to try and find multiple lists in a single method without exploding, since you'd need to surround each list call with a separate try/catch (or get an instance and check that it's not null).

This isn't a breaking change and simply re-implements the previous behavior of the Configuration class; the other alternative is overloading it with a default method, which is perhaps more useful: getStringList(path, default_list)

@chrisyco
Copy link

Oh, my bad. I guess I'll have to actually read the documentation before commenting...

@feildmaster
Copy link
Member

In all honesty... when "getting" something specific, it should always at least return what you're trying to get.

@codename-B
Copy link

Rather than this, getList(String key, List defaut); would be a preferred implementation of what you're after (I think)

@krinsdeath
Copy link
Contributor Author

I disagree on only one point: It's bull that I have to try/catch a get*List() call to prevent NPEs. There's something wrong in the implementing class, because that's where the NPEs appear. This fixes that.

While I do agree that a default list method is ideal, this still needs to be added.

@Dinnerbone
Copy link
Member

Thanks

@Dinnerbone Dinnerbone closed this Jan 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants