upgrade to guice 4.2.0 - Githubissues

rodionmoiseev commented 4 years ago

Hi, thanks. Sorry it took so long.

I committed your first set of changes. Not sure about the second force push, so I left it out.

Regards.

lelmarir commented 4 years ago

Hi, i'm sorry, that commit was intended for another repository, ignore it

but since you have seen it: I'm planning to use your library in a new project and my toolchain can't really handle gradle, so i'm converting it to maven; maybe both can live alongside or i can push only the other changes

rodionmoiseev commented 4 years ago

Hi, thanks for the comment. I see, I am glad to hear you are using the project!

Honestly I am not ready for maven maintenance, quite yet, so if you could help me with just the application itself for now that would be great 👍

kevinrobayna commented 4 years ago

But there’s no need, maybe it’s better to look into publishing the artefact in github as it is and then you can install it in your project however you wish.

https://help.github.com/en/packages/using-github-packages-with-your-projects-ecosystem/configuring-apache-maven-for-use-with-github-packages

On 7 Mar 2020, at 13:16, Rodion Moiseev notifications@github.com wrote:

Hi, thanks for the comment. I see, I am glad to hear you are using the project!

Honestly I am not ready for maven maintenance, quite yet, so if you could help me with just the application itself for now that would be great 👍

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

lelmarir commented 4 years ago

But there’s no need, maybe it’s better to look into publishing the artefact in github as it is and then you can install it in your project however you wish.

You're right, but i've found some "bugs" and missing features i need to implement (that i will share) and i'm more comfortable with maven

kevinrobayna commented 4 years ago

Yeah the thing is that gradle is more popular these days so it might be better for a open source tool to be in gradle :/

On 7 Mar 2020, at 17:43, Michele Preti notifications@github.com wrote:

But there’s no need, maybe it’s better to look into publishing the artefact in github as it is and then you can install it in your project however you wish.

You're right, but i've found some "bugs" and missing features i need to implement (that i will share) and i'm more comfortable with maven

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

lelmarir commented 4 years ago

Yeah the thing is that gradle is more popular these days so it might be better for a open source tool to be in gradle :/

don't get me wrong, but i think this is debatable, anyway i will try to push only fixes without maven related things

rodionmoiseev commented 4 years ago

Hi guys!

Maybe I am missing something, but I think you are talking about different things. I will quickly summarize my thoughts:

c10n build system: gradle or maven?

For compiling, testing and building/publishing packages, I would like to stick to gradle. I have nothing against maven, but I am not so enthusiastic about this system.

I understood that @lelmarir was offering to use maven build in parallel with gradle. This is possible, but would complicate maintenance, hence my response.

Package repository: maven central vs. github packages

Currently, I prioritize maven central. It makes it easy for others to use c10n as dependency, as maven central is well integrated into most build systems I know, including gradle and maven.

For some time, I was publishing to a custom-made repository directly in github (under the c10n-maven-repo/ folder). Honestly, I forgot why I really had to do that. Probably it was before I had publishing access maven central. This repository is very dependent on how Github exposes static resources, and afaik the URLs have changed a few times and are not stable. Now that Github is offering its own package repository system with disk quota, I would not be surprised if they restricted my custom-made repository all together. I will probably need to remove it sometime soon.

As for Github Packages, I took a quick look, and in conclusion it seems superfluous in the context of c10n project. For users that use my mainstream c10n dependencies, maven central should do the trick.

Individual repository needs

If you need to make fixes/updates to c10n and make them quickly accessible to your own projects, then I will recommend that you maintain your own fork and your own package repository, as @lelmarir seems to be doing. This way you can ensure you can deploy your fixes fast an in the way that works best for you.

I will try merge your fixes as fast as I can into the mainstream, but realistically I may not be able to build an official mainstream package with your fixes fast, as I have to verify the patch and prepare a release. Also, I may not accept the merge request as-is as it may conflict on some levels.

Conclusions for now

For the mainstream c10n project:

Build system will be gradle (I will eventually look into updating it to the latest version, as it's getting pretty old)
Package publishing will be maven central (via sonatype)
I will look into removing the c10n-maven-repo/ as it seems to be going against Github Packages project, and is pretty old and not bringing value afais

Thanks

lelmarir commented 4 years ago

I'm sorry to have caused problems, as i said, the maven thing push was a mistake, it was never intended to be in this repository. My other project uses maven, so I had to convert this to maven to allow me to do fast integrations with my IDE (can't mix maven and gradle projects). I understand that this project uses gradle and i have no intention to change it, but since i'm using maven anyway i let you know just in case.

rodionmoiseev commented 4 years ago

@lelmarir (cc: @kevinrobayna) no worries! I appreciate any constructive feedback! Let's close the topic for now.

If any of you would like to propose some new build management or package management for the future, please open a new issue and we can discuss it there. Cheers!

lelmarir commented 4 years ago

Hi, not sure if this is the right place, but i'd like to share with you that I've pushed my latest commit to my fork. Unfortunately right now i don't have time to create te push request (as i told you i neet to use maven and i don't have the time right now to filter it out). But I was to think that meanwhile i could share my changes so you can peek :)

rodionmoiseev commented 4 years ago

thanks! Will take a look later this weekend.

rodionmoiseev commented 4 years ago

@lelmarir Hi. I took a quick look at what you did in the last two commits. Here are my thoughts:

The ICU4J formatter works best as a custom message formatter using C10NConfigBase.setMessageFormatter(). The custom implementation of MessageFormatter can exist in your source base, so there is no need to modify C10N source, as far as I can tell. For Guice users you can do something like this (not tested):
```
   C10N.configure(new C10NConfigBase() {
        @Override
        protected void configure() {
            setMessageFormatter( new ICU4JMessageFormatter() );
        }
    });
    ... Guice.createInjector(C10NModule.scanAllPackages()).getInstance( MessageInterface.class );
```
With regard to automated Enum mapping binding for Guice, I do like the idea. After some thought, I think it would be better to achieve it by extending C10N like I have described in #47. Please take a look and tell me what you think 👍
Seems like you added a addC10NConfig method to C10NModule class, to extend C10N configuration. I think you should be able to equally do this by a prior call to C10N.configure( <your custom C10N config >), just like I did in the example above. Currently, Guice extension relies on the global static configuration object declared at the beginning of C10N class, because internally it's just making a call to C10N.get(<interface class>) static method. If you need to create multiple separately-configured C10NMsgFactory instances using C10N.createMsgFactory(), then this approach will not work. In that case you'll need to pass your own C10NMsgFactory instance to the scanAllPackages() method, and make it use that factory, instead of the global static root factory referenced by the C10N.get() method.

rodionmoiseev commented 4 years ago

Also it looks like you have moved guice extension source into its own top-level module (i.e. c10n/ext-guice/ directory. This is definitely a good idea 👍 . I should do the same.

lelmarir commented 4 years ago

The custom implementation of Message Formatter can exist in your source base

ok, no problem, i just thought it would be usefull in the base library.

I think you should be able to equally do this by a prior call to C10N.configure

Yes, but I don't really like to call a static config method inside a guice module (i know it will anyway), using a mode guice-oriented configuration seem more clean to me. I know that multiple modules would fail (they will use the same static instance); maybe i need to add a ceck to ensure only one exists.

lelmarir commented 4 years ago

Seems like you added a addC10NConfig method to C10NModule class, to extend C10N configuration

i almost forgot: I added the method because if i call C10N.configure as you suggest it would replace the previous config made by the module (or the module one would be replaced). Instead i need to extend the base configuration provided by the module (the enum binding) is this legit?

rodionmoiseev / c10n

upgrade to guice 4.2.0 #43