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

Dependency to org.apache.commons.lang3.StringUtils #6

Open
minduch opened this issue Aug 18, 2021 · 12 comments
Open

Dependency to org.apache.commons.lang3.StringUtils #6

minduch opened this issue Aug 18, 2021 · 12 comments

Comments

@minduch
Copy link

minduch commented Aug 18, 2021

Hi!

The dependency to org.apache.commons.lang3.StringUtils is not specified in the pom.xml file.
It is used in the AbstractParser last line of code:

  protected String buildVersion(String version, Matcher matcher) {
    version = buildByMatch(version, matcher);
    if (version == null) {
      return null;
    }

    version = version.replace("_", ".");
    return StringUtils.strip(version, " .");
  }

All the best!

@minduch
Copy link
Author

minduch commented Aug 18, 2021

Oh: any other dependencies you know about?

@minduch
Copy link
Author

minduch commented Aug 18, 2021

You should add:

<!-- https://mvnrepository.com/artifact/org.apache.commons/commons-lang3 -->
<dependency>
    <groupId>org.apache.commons</groupId>
    <artifactId>commons-lang3</artifactId>
    <version>3.12.0</version>
</dependency>

@minduch
Copy link
Author

minduch commented Aug 18, 2021

And then I found dependency to Maven artifact API with ComparableVersion, that in turn wishes (sometimes) to have SonaType. It becomes difficult...

@mngsk
Copy link
Owner

mngsk commented Aug 21, 2021

Are you having trouble compiling the project or do you just mean I should add the dependency explicitly for correctness sake? If that's the case you're absolutely right.

I didn't understand your comment about the ComparableVersion, though. Could you elaborate, please?

@minduch
Copy link
Author

minduch commented Aug 21, 2021

It's a little sad to require maven as dependency for ComparableVersion. So in my current version, I copied in the code for Maven ComparableVersion into your package, and made it package-private so that e.g. ProGuard can make it smaller. Then I removed the "import org.apache.maven.artifact.versioning.ComparableVersion;" as I had copied the code of ComparableVersion into that package. This will effectively remove the required dependency to the Maven library in pom.xml for runtime. I would see Maven dependencies for testing and building, not runtime! It's a typical build/testing tool.

I also removed the code for StringUtils.strip(String,String) with discreate code (because it's so simple) and to require commons-lang3 just for that single method call is "overkill". If you had used it everywhere and other functions in that library I would understand. Then I didn't have to modify pom.xml because there was no dependency of it.

I also updated the pom.xml for fasterjackson to latest version 2.12.4.

I have no trouble compiling it at all, not even 1.0.9, but 1.0.9 had a problem at runtime because commons-lang3 was not present in our classpath.

@minduch
Copy link
Author

minduch commented Aug 21, 2021

You might also want to update pom.xml with latest

            <artifactId>maven-gpg-plugin</artifactId>
            <version>3.0.1</version>

instead of version 1.6.

@mngsk
Copy link
Owner

mngsk commented Aug 21, 2021

Thank you for your insights!

When I started the project I decided to use ComparableVersion to avoid reinventing the wheel, but I agree it's a little awkward to depend on maven dependencies for runtime. Having it as a dependency is easier for me because I don't have to maintain it, and because I don't have any problem with the size of the library and its dependencies, but maybe you're right and it's time for this project to be more light weight. Would you be willing to help me out by keeping the class up to date with future patches from upstream?

Regarding the StringUtils.strip() method, I only used it because I already had it available through maven-artifact -> commons-lang3; I'm sorry I didn't add the dependency explicitly. But you're right, the code is simple enough and stable enough to incorporate it here.

I'd be happy to consider your changes, should you decide to make some pull requests.

@minduch
Copy link
Author

minduch commented Aug 21, 2021

Hi, yes I could help you out.

Could you "document" how you update the regex'es from the Matomo Device Detector project.
Would be nice if a it could be done using Maven or some Linux scripts to automate as much as possible, then do some manual programming work if required.

I was also thinking of making it an "Eclipse project", so adding e.g. .project and .classpath files, then somehow also document what this project version matches in comparison to the Matomo Device Detector project.

@mngsk
Copy link
Owner

mngsk commented Aug 21, 2021

Yeah, it would be great to automate the process.

Here is the script I use, but it's very basic. Just make sure both projects are up-to-date before running it.

Sometimes that's enough for all tests to pass, but other times the PHP version make some breaking changes that need to be ported, and those tend to be more time consuming.

I use eclipse myself, but I add the .project and .classpath files to .gitignore, because I don't like having IDE-specific stuff. And also is very simple in Eclipse to import an existing Maven project.

Hope this helps.

@minduch
Copy link
Author

minduch commented Aug 21, 2021

Would you mind to send me your email to christopher at mindus dot se?
I have a matter I wish to discuss, but not over github.

@mngsk
Copy link
Owner

mngsk commented Sep 9, 2021

I sent it the day after your comment, did you receive it?

@minduch
Copy link
Author

minduch commented Mar 13, 2022

I can't find the mail anymore... Please resend.

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

3 participants
@minduch @mngsk and others