jcabi / jcabi-manifests

Java library for convenient reading of MANIFEST.MF files available in classpath
https://manifests.jcabi.com
Other
60 stars 22 forks source link

`get(key)` Across Multiple Manifests #31

Open briantopping opened 8 years ago

briantopping commented 8 years ago

Cool library, thanks.

Would be great if the API could return a list of values for a single key, collected across all jars on the classpath. Should be pretty easy. I'd put in a PR, but I'll be doing mine in Scala :/

briantopping commented 8 years ago

In case you're marginally interested. I forgot some of my JavaConversions stuff...

object Main extends App {
  import scala.collection.JavaConversions._

  private val loader: ClassLoader = getClass.getClassLoader
  val paths = for {
    url <- loader.getResources("META-INF/MANIFEST.MF")
    manifest = { val m = new Manifests(); m.append(new StreamsMfs(url.openStream())); m }
    value <- Option(manifest.get("Manifest-Version"))
  } yield value

  paths.foreach(println)
}
dmarkov commented 8 years ago

@yegor256 dispatch this issue please, see par.21

yegor256 commented 8 years ago

@briantopping yeah, that's a good idea indeed, we'll try to implement it

dmarkov commented 8 years ago

@nesterov-n can you please help? Keep in mind this. If you have any technical questions, don't hesitate to ask right here; This task's budget is 30 mins. This is exactly how much will be paid when the problem explained above is solved. See this for more information

nesterov-n commented 8 years ago

@yegor256 For now we have Map<String, String> in Manifests.java and values with same key are ovewrriten. Can you help to find out is best way to implement

list of values for a single key, collected across all jars on the classpath

Should we use Map<String, List<String>> to store manifests values? It can broke backward compatibility. Maybe just add separate static methos in Manifests.java ? Something like this

public static List<String> collect(Collection<InputStream> manifests, Object key) {
     // collect and return multiple values here
}
yegor256 commented 8 years ago

@nesterov-n yeah, I think we should keep data in Map<String, List>, just like you suggested. we don't need to delete any existing public methods. just introduce a few new ones

nesterov-n commented 8 years ago

@yegor256 But class Manifests.java implements Map<String, String> interface. If we don't change interface to Map<String, List<String>> it's difficult to change underlying map

yegor256 commented 8 years ago

@nesterov-n why? internally you will have Map<String,List>, but the method will still expose Map<String,String>

nesterov-n commented 8 years ago

@yegor256 It gives lot of difficulty and dubious code. For example

private final transient Map<String, List<String>> attributes;
....
@Override
    public String remove(final Object key) {
        List<String> removed = this.attributes.remove(key);
        return removed != null ? removed.get(0) : null;
    }

And it's not most complicated case

How can i improve values() implementation?

@Override
    public Collection<String> values() {
        List<String> result = new ArrayList<String>();
        for (List<String> strings : this.attributes.values()) {
            result.add(strings.get(0));
        }
        return result;
    }

And Set<Map.Entry<String, String>> entrySet() has same problems

As for me this code is ugly. If you don't think so I can implement it

dmarkov commented 8 years ago

@nesterov-n the ticket is with you for 15 days already. If it is not finished in the next 48 hours, it will be re-assigned to someone else, see No Obligations principle, this article should help if you're stuck

-30 to your rating, your total score is -43

nesterov-n commented 8 years ago

@yegor256 Please see PR #33 It's contains variant with with static method returning multiple values without changing underlying map. Is it unacceptable?

dmarkov commented 8 years ago

@nesterov-n this task is taking too long, I have to change the performer, sorry. Please stop working with it right now. See our no obligations principle

-60 to your rating, your total score is -163

yegor256 commented 8 years ago

@dmarkov pls assign @nesterov-n back to this task

yegor256 commented 8 years ago

@nesterov-n pls keep this in mind: http://at.teamed.io/policy.html#10

dmarkov commented 8 years ago

@dmarkov pls assign @nesterov-n back to this task

@yegor256 there are already 2 active tasks in the project, we can't have more

dmarkov commented 8 years ago

@westonized it's yours now, please proceed keeping in mind our principles. Feel free to ask any technical questions right here in the ticket

Budget here is 30 mins (keep this principle in mind)

westonal commented 8 years ago

@dmarkov The task is ambigious and needs to be better defined. There are so many edge cases stemming from Manifest being a Map<String, String>. I can implement a method for getAll, with a backing field of Map<String, List<String>> but it is wholly unclear how this would work with the methods such as put and remove. It is definitely duel purpose, effectively maintaining two maps that I can't see anyone can cleanly implement.

westonal commented 8 years ago

@dmarkov I'm done, but I've raised two todo puzzles around my concerns, is it automatic that they get converted to new issues?

dmarkov commented 8 years ago

@dmarkov I'm done, but I've raised two todo puzzles around my concerns, is it automatic that they get converted to new issues?

@westonized yes, they will be converted to tickets automatically

westonal commented 8 years ago

@dmarkov Blocked by #38 as per discussion in PR #35

dmarkov commented 8 years ago

@dmarkov Blocked by #38 as per discussion in PR #35

@westonized #35 is a pull request, can't be an impediment

dmarkov commented 8 years ago

@dmarkov Blocked by #38 as per discussion in PR #35

@westonized got it, let's wait for #35, #38

proshin-roman commented 6 years ago

When I found this library and tried to use it I was suprised that it always finds attributes through all available manifest files in the classloader. I - as a user - would expect another behaviour that this library just reads MANIFEST.MF from my war/jar file and - if I add some special option - can work with other manifest files. I might be wrong but I don't see any use case to have attributes of "internal" manifest files - from libraries etc. So the whole this issue with reading/storing multiple values for the same attribute might be a wrong way.

Simple use case (that's what I faced with):

So I see two possible ways to improve the user experience:

  1. By default library read only the "top-level" manifest file and by adding some special option it can read other files.
  2. Library always reads all manifest files but groups them by packages. Then you can extract the value that you need just by specifying package of your app/library.