StackWizardSettings breaks java.util.Map contract about cjwizard HOT 22 CLOSED

@stevesobol, as I've said, I think that WizardSettings should not extend Map at all, since not all implementations are valid Map implementations. We can keep some of the Map's methods declared in WizardSettings to avoid breaking too much, but we need to make sure those methods' contracts fit all known implementations (for example, remove works differently for StackWizardSettings).

We should implement those missing methods. This seems, to me, to be the path forward. It doesn't involve making breaking changes to the code.

This doesn't break binary compatibility, but it does break the behavior (again, StackWizardSettings.remove would have to change), which to me sounds even worse - this change won't be caught by the compiler!

from cjwizard.

@nigredo-tori I am still here FYI :) trying to find time to work on this, am working on it today and I'm thinking we get all of these things done, including the convenience methods, and since we have major changes, the next release will be 2.0.0 instead of 1.0.10 as I'd planned. There are still things I need to get done (much better docs, a real website, unit testing). But they can wait imho.

from cjwizard.

Hello!

I wasn't involved in the project when #46 was opened (or closed), so I have no response to that, but please let me take a look at this and see what I can do about it. Thanks.

from cjwizard.

maybe it would be better if it just extends HashMap, that would probably fix this and enable us to delete a bunch of code

from cjwizard.

ehh on second though, i'm not sure that suggestion would work.

from cjwizard.

I'm going to dig into this this coming week. @spyhunter99 if you have any ideas, I want to hear them and could you please do me a favor... email me at [email protected] from an email address you would like to use to communicate about this project. You've emailed me in the past but I have no idea if I still have those emails. And we may need to discuss things that don't make sense to discuss here :) thanks

from cjwizard.

So, @spyhunter99 opened #46 and then immediately closed it, but his comment about the StackWizardSettings not being for user settings (? not sure exactly what that means) doesn't seem relevant. StackWizardSettings implements WizardSettings, which extends Map. We should implement those missing methods. This seems, to me, to be the path forward. It doesn't involve making breaking changes to the code. Thoughts? @nigredo-tori?

from cjwizard.

Ok. Let me get to work on this - thanks for your feedback

from cjwizard.

so... I have Wizard Settings not implementing Map anymore and there are a few methods I have to implement

from cjwizard.

get, put andkeySet were all already implemented in StackWizardSettings, so it was just a question of adding them to the WizardSettings interface.

from cjwizard.

FlatWizardSetting inherits from HashMap and broke because it didn't implement the get method. This was my solution:

	@Override
	public Object get(String key) {
		return super.get(key);
	}

from cjwizard.

@nigredo-tori I haven't opened a pull request yet. I also haven't tested the changes yet. But I'd like you to look at them.

[branch deleted]

from cjwizard.

@stevesobol, off the top of my head:

1. StackWizardSettings.entrySet and StackWizardSettings.values are not implemented, and should be removed.
2. WizardSettings.put should not be called "an optional operation" in its javadoc - unless there is a valid use case for immutable WizardSettings implementations.
3. Few of the methods in StackWizardSettings use Object rather than String as the key type (get, containsKey, remove, removeAll); since we're breaking a lot of stuff anyway, we can fix these too.

Also, I can think of a few convenience Map methods that can be reimplemented in WizardSettings as default methods but this will have to wait until we drop Java 7 support. Same with conversion from WizardSettings to Map<String, Object>.

from cjwizard.

Haven't forgotten about this. Things have been crazy and I've had to focus on getting new work.

from cjwizard.

I'm starting clean. I'm not sure exactly where I was with the changes. The branch I linked to earlier is gone, so I'm going to remove the link to it.

from cjwizard.

I'm going to have to update WizardSettings too, as StackWizardSettings extends that class and it extends Map.

from cjwizard.

I think I'm going to have to grab the source code for Map and basically create a duplicate class, minus the stuff we do not need. Too many methods to rewrite otherwise.

Thank $DEITY for OpenJDK.

from cjwizard.

@nigredo-tori hm. OpenJDK is GPL-licensed which means that we'd also need cjwizard to be GPL licensed if I was to snarf the source code for Map and modify it - and cjwizard has always been Apache and I'd prefer to continue using Apache or switch to MIT. Either license is much more permissive than GPL. Thoughts on how we can deal with these changes? If I don't have WizardSettings extending Map, there are a bunch of useful methods that go away...

from cjwizard.

@stevesobol,

I think I'm going to have to grab the source code for Map and basically create a duplicate class, minus the stuff we do not need. Too many methods to rewrite otherwise.

Please don't do that. Reimplementing library classes is generally not a good idea. In fact, this very issue is about StackWizardSettings incorrectly implementing Map. 😄

It should be easier and cleaner to solve this with aggregation. For example:

public interface WizardSettings {
  void rollBack();
  void newPage(String id);
  void commit();

  /** Get a {@code Map} with settings for the current page.
    * Mutating the resulting map will mutate the settings.
    */
  Map<String, Object> getCurrentPageSettings();

  /** Build a {@code Map} with settings for all the pages.
    * Mutating the resulting map will not mutate the settings.
    */
  Map<String, Object> buildFullSettings();

  // We can provide convenience wrappers for methods we use the most.

  /** Put a key-value pair into the current page settings.
    * @return previous value for this key on the current page, or {@code null}.
    */
  default Object put(String key, Object value) {
    return getCurrentPageSettings().put(key, value);
  }

  /** @return the value for the key in full settings, or {@code null}. */
  default Object get(String key) {
    return buildFullSettings().get(key);
  }
}

I don't like getCurrentPageSettings returning a mutable backing map - this constrains the implementation too much. However, since this whole interface is written in the mutable style, this should do for now.

from cjwizard.

from cjwizard.

does this help? #82

from cjwizard.

I'm sure it will. I'll check it out asap.

I am so sorry, guys. I'm being stupid about this. Lots going on over here, but that's no excuse.

from cjwizard.