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

u3d/list: display full revision number (prepares for #274) #280

Merged
merged 2 commits into from
Apr 19, 2018

Conversation

lacostej
Copy link
Member

@lacostej lacostej commented Apr 18, 2018

Pull Request Checklist

  • My pull request has been rebased on master
  • I ran bundle exec rspec to make sure that my PR didn't break any test
  • I ran bundle exec rubocop to make sure that my PR is inline with our code style
  • I have read the code of conduct

Pull Request Description

Tested on Linux/Windows/Mac on the computers I had access to, would need slightly more testing to make sure Unity doesn't do weird things with special builds. So adding the feature as part of list to test it a bit more.

We need to work a bit more on the overall feature set to see how to solve #274

Mac:

image

Linux:

image

Windows:

Meh... believe me it worked.

@@ -163,6 +167,39 @@ def plist
end
end

class LinuxInstallationHelper
STRINGS_FULL_VERSION_MATCHER = /^[0-9\.abfp]+_[0-9a-f]{12}/
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I would have to check if we get weird values, e.g. with some of the versions that had weird build numbers.

@niezbop
Copy link
Member

niezbop commented Apr 19, 2018

It works nicely on Windows :)

Version 4.7.1f1       [68064779abd0]  (C:/Program Files/Unity_4.7.1f1)
Version 5.0.0f4       [5b98b70ebeb9]  (C:/Program Files/Unity_5.0.0f4)
Version 5.3.6f1       [29055738eb78]  (C:/Program Files/Unity_5.3.6f1)
Version 5.6.0f3       [497a0f351392]  (C:/Program Files/Unity_5.6.0f3)
Version 5.6.1f1       [2860b30f0b54]  (C:/Program Files/Unity_5.6.1f1)
Version 5.6.5f1       [2cac56bf7bb6]  (C:/Program Files/Unity_5.6.5f1)
Version 2017.1.2f1    [cc85bf6a8a04]  (C:/Program Files/Unity_2017.1.2f1)
Version 2017.2.0f3    [46dda1414e51]  (C:/Program Files/Unity_2017.2.0f3)
Version 2017.2.1f1    [94bf3f9e6b5e]  (C:/Program Files/Unity_2017.2.1f1)
Version 2018.1.0b2    [79c3bdce0980]  (C:/Program Files/Unity_2018.1.0b2)

Copy link
Member

@niezbop niezbop left a comment

Choose a reason for hiding this comment

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

Minor nitpicking regarding the installation helpers. Feel free to merge without though

@@ -176,6 +213,10 @@ def version
version
end

def build_number
@build_number ||= LinuxInstallationHelper.new.find_build_number(root_path)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could make the find_build_number a class method rather than an instance? It would make a bit more sense to me. Especially given the fact that the Helper doesn't carry any sort of data so all of its current methods could be moved to self?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to avoid class methods! I don't find them particularly elegant from an API point of view. Thoses classes are meant internal though, not part of the public API in my mind.

@lacostej lacostej merged commit 4964aa3 into DragonBox:master Apr 19, 2018
@lacostej lacostej deleted the revision branch April 19, 2018 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants