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

One more method is needed in License #60

Closed
nikolayo opened this issue Oct 3, 2019 · 16 comments
Closed

One more method is needed in License #60

nikolayo opened this issue Oct 3, 2019 · 16 comments

Comments

@nikolayo
Copy link

nikolayo commented Oct 3, 2019

Great library. Thank you. The License class though needs IMHO the following method :

    /**
     * Get an unmodifiable map of feature names to features.
     * 
     * @return the name to feature map
     */
    public Map<String, Feature> getFeatures() {
        return Collections.unmodifiableMap(features);
    }

There are multiple use cases for this. Let me mention just as an example conversion to JSON.

Regards
Nikolay

@verhas
Copy link
Owner

verhas commented Oct 5, 2019

I see your point. I always thought of a license as a set of features, where the features are a fixed set, like user count, processor id, max allowed memory whatever. The proposed function is useful in case the using application does not know a-priory the possible feature names. What is the use for that? Why it the use to convert a license to JSON?

@nikolayo
Copy link
Author

nikolayo commented Oct 6, 2019

I use the conversion to JSON for the purpose of displaying a license in a web - based UI. But frankly speaking the motivation for this proposal is more of a "gut feeling" that without this method the design of License is somehow incomplete.

@verhas
Copy link
Owner

verhas commented Oct 6, 2019

I understand your point. I never thought of any need for the features to be enumerable.

On second thought though: if they are enumerable then the order of the features may also be important. How about just opening the private Feature[] featuresSorted(Set<String> excluded) method to be public instead of the new method?

@nikolayo
Copy link
Author

nikolayo commented Oct 6, 2019

I can see how opening featuresSorted to the public can be functionally equivalent to my proposal and why you may want to reuse an existing method rather than create a new one. With that said - to my (biased my my own proposal) eye getFeatures with its simplicity seems somewhat more "aesthetically pleasing" than featuresSorted but at the end of the day it is up to you.

@nikolayo
Copy link
Author

nikolayo commented Oct 6, 2019

P.S.: Maybe I should explain a bit more : from the standpoint of the general public featiresSorted may seem as a bit of overdesign because in the general case there is no need for sorting or/and exclusion and in the special cases where such need exists it can be satisfied externally.

@verhas
Copy link
Owner

verhas commented Oct 6, 2019

You are absolutely right. I will need some time to consider and digest the different drivers.

How fast do you need this feature? I guess you can go with your own modified version for a while, but of course, you would like to have a new release with the feature some way or the other so that your branch maintenance burder is limited in time and is contained.

@nikolayo
Copy link
Author

nikolayo commented Oct 6, 2019

Oh, I do this for the sake of the library and for the benefit of the general public only. :)

I personally intend to use the library in a project which is Java 8 based. So I had to "downgrade" it by hand from Java 11 and to use it as source in my project. There I have already implemented the getFeatures method.

So thank you for your kind understanding but schedule is entirely up to you as far as I am concerned.

@nikolayo
Copy link
Author

nikolayo commented Oct 6, 2019

P.S. : You could of course make easier the life of people like me still living on Java 8 (in the corporate world) by not using Java 11 but that is already not something I would venture to suggest. You know better how you want to position your library/project in the world of Java versions.

@verhas
Copy link
Owner

verhas commented Oct 7, 2019

I created a release 3.1.3 just a minute ago. This release contains the method you requested, although the implementation is not exactly what you suggested.

In return, I would like to ask you a favor. Please check that this release binary works nicely with Java 8 JDK and JVM8 that you use. I configured JBel to create JVM8 class files from Java 12+ source files so that this release should work nicely with Java 8, but this is not a native Java 8 compilation, so the proof of the pudding is eating it.

Please give me feedback, if you can, here and then I will close this ticket.

@nikolayo
Copy link
Author

nikolayo commented Oct 7, 2019

Thank you for your quick action. The 3.1.3 release appears to work well with Java 8. What I did more specifically in order to test that was :

  1. I checked out your 3.1.3. tag and installed it in my local maven repository using Java 11 for the build.
  2. I removed from my project the License4J sources and instead added a dependency on your 3.1.3 in the pom.xml.
  3. I added to my project the sources of your tests and all related pom features.

After all of the above I switched back to Java 8 and ran through Maven the tests which all passed. Since the tests were compiled with Java 8 I assume that everything is OK.

Now if you also upload this version to the Maven central repository, I will be able to consume it as a dependency from there.

Thank you once more.

Regards
Nikolay

@nikolayo
Copy link
Author

nikolayo commented Oct 7, 2019

P.S. Sorry, I forgot to mention that the tests I used were from my "downgraded" to Java 8 version of your project.

@verhas
Copy link
Owner

verhas commented Oct 7, 2019

emoved from my project the License4J sources

you mean License3j

Now if you also upload this version to the Maven central repository...

actually it is. It was already when I commented. It needed or it still needs, probably some time to synchronize from the release stage to the central. It is usually less than 10 min, so probably it is already there.

@verhas verhas closed this as completed Oct 7, 2019
@nikolayo
Copy link
Author

nikolayo commented Oct 7, 2019

I can see now 3.1.3 in the Maven central repository.
Thank you. Bye.

@nikolayo
Copy link
Author

BTW, if you use in getFeatures TreeMap, it will do the sorting for you. Something like this :

    public Map<String, Feature> getFeatures() {
         return Collections.unmodifiableMap(new TreeMap<String, Feature>(features));
    }

@verhas
Copy link
Owner

verhas commented Oct 19, 2019

Did you actually check it? There may be some things to consider about how features are compared. The current implementation uses an external comparator and does not need the Feture class to be comparable.

@nikolayo
Copy link
Author

nikolayo commented Oct 20, 2019

The keys are strings, so I see no problem with the TreeMap keeping them in order. I have not tested the specific method but I have used TreeMap with String keys elsewhere without problems in ordering the keys.

TreeMap also has a constructor TreeMap(Comparator<? super K> comparator) but for String keys this is really redundant because String implements Comparable .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants