Skip to content
This repository has been archived by the owner on Jul 19, 2018. It is now read-only.

Java Parser: Fixes issue #374 with OS $1 replacement #376

Closed
wants to merge 1 commit into from

Conversation

commenthol
Copy link
Collaborator

This fixes the issue #374 within the java parser.

@@ -4,7 +4,7 @@
<groupId>ua_parser</groupId>
<artifactId>ua-parser</artifactId>
<packaging>jar</packaging>
<version>1.3.0-SNAPSHOT</version>
<version>1.3.1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're on a snapshot version anyways.
Why do we need to increase the version number?

@commenthol commenthol changed the title Fix for Issue #374 Java Parser: Fixes issue #374 with OS $1 replacement Apr 19, 2014
@commenthol
Copy link
Collaborator Author

For your remarks on "*.contains("$1") please check:
https://github.com/tobie/ua-parser/blob/master/java/src/main/java/ua_parser/UserAgentParser.java#L94
as well as
https://github.com/tobie/ua-parser/blob/master/java/src/main/java/ua_parser/DeviceParser.java#L88
I think there is a reason to keep this.
If I might be wrong, please issue a PR.
With regards to the versioning I changed to the release version 1.3.0.

@somechris
Copy link
Contributor

I changed to the release version 1.3.0.

The java part already has seen a version 1.3.0 (see 7239f0b).

If you think a separate release for the java version is needed, I'd split that out to separate
commits, followed by a "bump to SNAPSHOT" commit as was done with previous releases:

But is this tiny change worth a version change at all?

@commenthol
Copy link
Collaborator Author

Is any change worth a version change? Please tell me what you want me to do.
A simple "Stay on version 1.3.0-SNAPSHOT" would do.

@somechris
Copy link
Contributor

I have no say here, but yes: staying on version 1.3.0-SNAPSHOT would make most sense to me.

@commenthol
Copy link
Collaborator Author

Sorry for this. Now it should have the right version.

@somechris
Copy link
Contributor

Looks good to me now. Verified that tests now pass.

(My inline comments are only nits anyways)

@rbywater
Copy link

In terms of version I'd actually suggest it should 1.4.0-SNAPSHOT. According to the Git log, the version in the pom went to 1.3 and then back to 1.3-SNAPSHOT. There's also a tag already for java-1.3.0 so it would seem safest to bump the version up to avoid any issues where people have already picked up 1.3.0 as being "official".

@somechris
Copy link
Contributor

According to the Git log, the version in the pom went to 1.3 and then back to 1.3-SNAPSHOT

Yes, and ua-parser has always put the SNAPSHOT version after the release:

  • 1.2.2
  • 1.2.2-SNAPSHOT
  • 1.3.0
  • 1.3.0-SNAPSHOT

ua-parsers approach may be unusual to some, but (as no release plugin etc is in place) it's
not a problem either. Maven can cope with that just fine.

Trading the x -> x-SNAPSHOT -> (x+1) -> (x+1)-SNAPSHOT scheme for
x-SNAPSHOT -> x -> (x+1)-SNAPSHOT -> (x+1) sounds good, but versioning is
unrelated to issue #374 that this pull request covers.
So I'd put version changes in a separate pull request.

@somechris
Copy link
Contributor

So I'd put version changes in a separate pull request.

I've done so in pull request #380

@rbywater
Copy link

Yes sorry - didn't mean to hijack the pull request! Thanks for putting in #380 - saves me jumping in to do the same.

@tobie tobie removed the java label Aug 15, 2014
@Ironholds
Copy link
Collaborator

#434 I believe handles the same thing.

@Ironholds Ironholds closed this Aug 29, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants