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

Generate models automatically to keep them up-to-date #4

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ainame
Copy link

@ainame ainame commented Jul 20, 2021

Prior to this PR, xcresult gem has been using (semi?) handcrafted models to parse .xcresult data, I assume. So that it's not up-to-date to the latest version nor implementing all of the properties on each type.

This PR aims to switch the strategy to maintain xcresult to using a script to generate models based on what xcrun xcresulttool formatDescription get --format json outputs.

As a result, we've got new models related to ActivityLog (honestly I don't know what it is though😅) and most importantly it's got "version" info and the timestamp of the last update in its code.

With this PR, the file structure related to models will be like below. (Naming suggestions are very welcome🙏)

  • lib/xcresult/models.gen.rb - automatically generated file that defines models and is supposed to be updated by bin/generate_models script
  • lib/xcresult/models.rb - entrypoint to do require models.gen and add convenient methods to defined models

bin/generate_models script is the one we can use to update lib/xcresult/models.gen.rb. We might be able to automate to create PRs to keep models up-to-date in future but for now, we need to run it manually.

One thing I need to mention is that this time I wrapped all models into another nested namespace XCResult::Models to make it easy to differentiate other classes that exist other than representing models. And you won't need to worry about the conflict of naming with what Apple gives us in the future. I started using dynamic class loading like Kernel.const_get("XCResult::Models::#{type_name}"), which makes implementing polymorphic objects easier a lot. If we don't use a dedicated namespace, it's less likely but may happen to load the wrong classes. It's safer to assume classes under XCResult::Models namespace are for models parsing the JSON.

This is a breaking change but I would justify it for the following reasons, in my opinion.

  1. This gem hasn't reached v1.0 so semantically it can include breaking changes anytime according to semantic versioning rules
  2. In reality, most users don't care what classes this gem internally uses with usages like in README; i.e. parser.export_xccovreports(destination: './outputs') https://github.com/fastlane-community/xcov/blob/f8904c7f000b415c7800b1c68c50e8de9dab76da/lib/xcov/manager.rb#L217-L221
  3. Even if it breaks something, required migration is super easy (appending ::Models) and only once but the benefit will be forever

I can easily remove the additional nest of the namespace (::Models) if you are concerned about compatibility. Please let me know🙂


JFYI: After merging this PR, I'd like to add screenshot extraction functionality to this gem 🏃

@ainame ainame marked this pull request as ready for review July 21, 2021 09:52
@ainame ainame mentioned this pull request Jul 23, 2021
@ainame
Copy link
Author

ainame commented Aug 21, 2021

I realised that xcrun xcresulttool formatDescription get --format json outputs JSON-schema so it would be easier to parse than markdown and parsing code is cleaner😂 This PR works fine but will revise it with JSON schema later.

@ainame
Copy link
Author

ainame commented Nov 24, 2021

I switched the input format to JSON so that parsing is easy now. However, I decided not to go with keeping markdown comments (original definition) as it's just duplicated and now a bit difficult to replicate😅

Updated the models with stable Xcode 13.1's xcresulttool accordingly.

@lechuckcaptain
Copy link

Hey @ainame, super interested in this and #5 PRs. Can I ask what's the status on this and if there is any blocker or help required?

@ainame
Copy link
Author

ainame commented Feb 28, 2022

Nothing blocks at all. It's just a matter of who to review and merge, I guess?
@joshdholtz might be busy but let's ping him here just in case.

@lechuckcaptain if you could test #5's branch with your project and report feedback, that would be amazing, too!

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

Successfully merging this pull request may close these issues.

2 participants