[Toolkit Cleanup] Yard doc: step 1 – config cleanup #198

AliSoftware · 2021-02-01T15:02:17Z

First step to add YARD documentation #192.
Also starts to also address #194 (README/CONTRIBUTING doc).

Adds some basic doc to configure your IDE and dev environment for ruby
Configure the rspec and yard gems properly
Adds documentation for *_version_helper.rb files

To Test

This is typically quite hard to test as automatically and strictly, as this is mainly about documentation, so it will be a lot of manual proof-reading.

Proof-reading the documentation

Run bundle install to install the gems, especially yard.
Run bundle exec yard stats --list-undoc lib/fastlane/plugin/wpmreleasetoolkit/helper/**/*_version_helper.rb to see if there is any methods or constants missing documentation in those 2 files (ios+android version helpers)
- The only expected documentation missing should be about modules Fastlane and Fastlane::Helper, for which I didn't think it would make sense to add specific documentation, but all the rest of those 2 files should be documented
Run bundle exec yard doc to generate the documentation.
- Ensure that there is no warnings or errors printed in the stdout while generating the documentation.
- Open the yard-doc/index.html file, and proof-read the documentation, especially for AndroidVersionHelper and IosVersionHelper modules, from within your browser.
- For each method in those documented modules, ensure that the method description, parameter types and descriptions, and return types make sense and are consistent with the implementation of those methods

💡 I highly encourage you to review the added documentation in the yard-doc/index.html page of your browser (see above) rather than / in addition to reviewing it directly from the diff in GitHub, both because that would ensure that the rendering is correct once rendered as HTML, but also because I have to admit I had my eyes focused on VSCode and the IDE when writing the code and didn't check every single rendering in the HTML doc myself, so having some fresh eyes looking at it from that side would allow a different perspective.

Double-checking the methods that have been renamed

This PR also contains some very small renaming of some methods. It's not a big refactoring but more making some methods conform to ruby conventions a bit more.

But because ruby is a duck-typed language without type safety like for compiled languages, and because we don't yet have rspec (unit tests) for those *_version_helpers methods yet (that's for #193 to address) and CI won't help here, I would appreciate a thorough code consistency review / confidence check for those method renamings here, i.e. double-check that when I renamed a method, I didn't forget renaming all its call sites too (the best I could do on my end is a project-wide textual Find in VSCode… which is quite error-prone…)

Notes

Incremental PRs and WIP

This is the first PR in a series of incremental PRs to clean up the codebase, especially add documentation and tests.

This PR targets the branch toolkit-cleanup, which was just cut from develop. I intend to stack all my upcoming incremental cleanup PRs on top of this toolkit-cleanup branch, and only merge toolkit-cleanup to develop once the sum of all incremental PRs feels stable enough, to avoid merging unfinished cleanup and WIP into develop directly.

Confusing names and refactoring

While documenting, I came across some strangely/confusingly named methods especially in the Android helpers (e.g. calc_next_release_version which returns a -rc- beta version and not a release version name, because it's in fact the name to use after the release cut… but the method name is quite confusing. Some others are also not super clear). Which made the codebase quite hard to decipher and understand at first.

I plan to do some renaming of those to add more clarity, but probably only once I've added a couple of unit tests on those first before refactoring everything (especially given how ruby is not compiled nor typed, making a refactoring quite risky without writing any unit test first).

AliSoftware · 2021-02-01T18:24:47Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_version_helper.rb

+        # @param [Hash] alpha_version The current alpha version hash, with keys `"name"` and `"code"`, or nil if no alpha version
+        #
+        # @return [Hash] The hash containing the version name and code to use after release cut
+        #
        def self.calc_next_release_version(version, alpha_version = nil)


This one confused me quite a bit… method named "next release version" but in fact it's returning a "X.Y-rc-1" version… so more the name of the "first beta after code freeze" rather than the "release version".

This is why the documentation was also so confusing to write at first. Will probably rename in a separate/future PR.

AliSoftware · 2021-02-01T18:26:11Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_version_helper.rb

-                if (line.include? keyword)
-                  return line unless line.include?("\"#{keyword}\"") or line.include?("P#{keyword}")
+                if line.include?(keyword) && !line.include?("\"#{keyword}\"") && !line.include?("P#{keyword}")
+                  return line.split(' ')[1]


All call sites were doing the split(' ')[1], so I figured I'd DRY it here – which actually makes more sense so that the method returns the keyword value, and not the ".gradle line containing the keyword, with potentially some indentation and spaces in front"

AliSoftware · 2021-02-01T18:27:31Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_version_helper.rb

                    version_name = line.split(' ')[1].tr('\"', '')
-                    line.replace line.sub(version_name, version[VERSION_NAME].to_s)
+                    line.sub!(version_name, version[VERSION_NAME].to_s)


While String#sub returns a new string in ruby, String#sub! modifies the string in-place. So more idiomatic and performant than calling replace on the same string with the returned value like before.

AliSoftware · 2021-02-01T18:45:44Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_version_helper.rb

        def self.calc_next_hotfix_version(hotfix_version_name, hotfix_version_code)
-          { VERSION_NAME => hotfix_version_name, VERSION_CODE => hotfix_version_code}
+          { VERSION_NAME => hotfix_version_name, VERSION_CODE => hotfix_version_code }


This one was also quite confusing, name suggesting we compute the next hotfix so that we will increment the versionName and code… but in practice we're not. Which matches how it's used at call site, but is damn confusingly and misleadingly named.

Will probably either rename the method to a more apt name in a subsequent PR, or more likely try to match ios here and update the call site to let this method do the hotfix increment as its name suggests, instead of making the increment at call site. That will require some refactoring of the Action tho, which is why I kept that for a future, separate PR.

jkmassel

This generally looks good – I left some nitpicks for your consideration (as well as a few suggested fixes).

One global note – the use of periods to demarcate sentences vs not – it seems somewhat inconsistent, but also it's IMHO a silly thing to get too caught up in and spend tons of time on.

I took a look at the method renaming and nothing seemed out of place there – I also checked in both WPAndroid and WooAndroid and neither had references to this version (though WPAndroid has its own copy of the Android version helpers file...)

I don't foresee this requiring a re-review, so feel free to merge when you feel the feedback is sufficiently considered and/or addressed.

.yardopts

jkmassel · 2021-02-01T19:14:04Z

Gemfile

 gem "danger", "~> 8.0"

+group :test do


Nice cleanup

jkmassel · 2021-02-01T19:19:29Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_version_helper.rb

      module AndroidVersionHelper
+        # The key used in internal version Hash objects to hold the versionName value


These were rather unclear to me when reading the docs on their own (it's much more clear in context of reading the whole file).

For the purposes of documentation, WDYT about adding an example for each (or is there a way to make them internal to this module and thus not require public documentation?) Understanding how these fit into the context might help?

That said, I could see some of this being replaced a level lower by a parser, and thus only operating on a higher-level abstraction, so take it with a grain of salt?

Also from a consistency point of view – all of the method docs end in a . (like a sentence) where these don't– not sure if intentional though 🙂

I already have on my list to get rid of those in a future PR and replacing them either with using symbols as keys as it's customary in Ruby (:name and :code, and don't bother having constants for them), or alternatively create a Version = Struct.new(:name,:code) type and use it everywhere in place of a Hash. But preferred to keep all that for a future PR than cram everything in here.

Also a Android::Version object with name and code properties + instance methods would probably make more sense than using a Helper in the first place, something like:

class Version # [String] Doc here attr_reader :name # [String] Doc here attr_reader :code # @return [Version] Next release version def next_release … end … end

(Or maybe make the internal properties be :major, :minor, :hotfix + alpha? and suffix, and make def name be computed from all that to assemble the parts; which would make all the internal math of incrementing values even easier)

That kind of change will make it easier to document without having to repeat the documentation for "name" and "code" and the Hash everywhere, and would feel much more idiomatic. That one is on my list, not sure when I'll get to it but definitively part of the plan.

Tracked in #203

lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_version_helper.rb

jkmassel · 2021-02-01T20:11:51Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_version_helper.rb

      module IosVersionHelper
+        # @!group The indices for various parts of the version parts array.


This is a different heading than on Android – it kind of helps clarify things, though same note applies for your consideration.

I actually ended up removing it, as (1) it doesn't add much value in the generated doc, and (2) it will likely disappear anyway once I get to #203

jkmassel · 2021-02-01T20:13:50Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_version_helper.rb

+        #
+        # @param [String] version The current version to create an internal version name for.
+        #
+        # @return [String] The internal version, in the form of "X.Y.Z.YYYYMMDD".


Not necessarily important to this PR, but this assumes that we never have more than one beta in a day. Regrettably, this assumption is incorrect 😅

Might be worth addressing down the road?

Tracked by #202

jkmassel · 2021-02-01T20:14:35Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_version_helper.rb

+        #
+        # @param [String] new_version The new value to set the `app_version` entry to.
+        # @raise [UserError] If the Deliverfile was not found.
+        #
        def self.update_fastlane_deliver(new_version)
          fd_file = "./fastlane/Deliverfile"


This path seems mildly fragile, but not necessarily the job of this PR to fix it.

jkmassel · 2021-02-01T20:16:40Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_version_helper.rb

+        #
+        # @param [String] version The version string to split into parts
+        # @return [Array<String>] An array of exactly 4 elements, containing each part of the version string.
+        # @note If the original version string contains less than 4 parts, the returned array is filled with zeros at the end to always contain 4 items.


This answered my question about the line above as soon as I thought of it. Nice!

jkmassel · 2021-02-01T20:18:15Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/promo_screenshots_helper.rb

@@ -282,8 +282,8 @@ def draw_text_to_canvas(canvas, text, width, height, x_position, y_position, fon
      #
      # @example
      #
-      #.  image = open_image("image-path")
-      #.  mask  = open_image("mask-path")
+      #   image = open_image("image-path")


Thanks for cleaning up my mess here 😂

mokagio

Note: given @jkmassel already review this, I only gave it a quick pass.

Look good. Thank you for doing this. I bet you have a much better understanding than I do of how the toolkit works now. And I'm grateful I'll be able to benefit from it by reading the documentation if I'll land on this code in the future 🙇‍♂️

As an aside, looking at some of the undocumented code I noticed some => Object when the return type is actually a String, e.g.:

That's obviously due to missing documentation and is not an error per-se, but it made me miss the types I got so used to with Swift, so I opened this issue for discussion.

mokagio · 2021-02-02T04:10:58Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_version_helper.rb

+        #
+        # @return [String] The predicted next version, in the form of "X.Y".
+        #         Corresponds to incrementing the minor part, except if it reached 10
+        #         (in that case we go to the next major version, as decided in our versioning conventions)


I remember searching for a link to these conventions but without luck. The best I could come up with was the WordPress versions page.

mokagio · 2021-02-02T04:11:29Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_build_prechecks.rb

@@ -14,7 +14,7 @@ def self.run(params)
        beta_version = Fastlane::Helpers::AndroidVersionHelper.get_release_version() unless !params[:beta] and !params[:final]
        alpha_version = Fastlane::Helpers::AndroidVersionHelper.get_alpha_version() unless !params[:alpha]

-        if (params[:final] and Fastlane::Helpers::AndroidVersionHelper.is_beta_version(beta_version))
+        if (params[:final] and Fastlane::Helpers::AndroidVersionHelper.is_beta_version?(beta_version))


Yey for using Ruby's ? and !s method suffixes.

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_version_helper.rb

mokagio · 2021-02-02T04:11:42Z

.gitignore

+test_results/
+coverage/
+
+# YARD Documentation


+1 for writing "YARD" with all upper case letters, because it's an acronym (Yay! Another Ruby Documentation tool) 🤓

Yeah, I like going the extra (0.00056818) mile by using correct case 🥁 😆

Instead of guessing a possibly wrong default value

AliSoftware added 14 commits February 1, 2021 12:44

Update CONTRIBUTING.md + rspec & yard default opts

729ea28

[yard] ios_version_helper

85656d6

Ruby convention for bool-returning methods

6aaa05d

small renames

ed58b2d

nit refactorings

0091bb5

Fix @example tags in yard doc

229a8d5

Fix @throws -> @raise yard tags

5d64c7b

yard nit fixes

6fd7b24

[yard] android_version_helper

635249c

[yard] ios_version_helper (nits)

0da5978

yardopts @env

3296e3d

Improve @env documentation

749e7a2

[yard] android_version_helper: constants

024d971

[yard] android_version_helper (finalize)

05ddd9e

AliSoftware requested a review from a team February 1, 2021 15:02

AliSoftware self-assigned this Feb 1, 2021

AliSoftware requested review from oguzkocer, jkmassel and mokagio and removed request for a team February 1, 2021 15:02

Fix parameter names in screenshots_helpers

e52ec0d

AliSoftware mentioned this pull request Feb 1, 2021

[Toolkit Cleanup] Yard doc: step 2 – rename module Helpers -> Helper #199

Merged

formatting fix

4ccc052

AliSoftware changed the title ~~[Toolkit Cleanup] Yard doc: step 1~~ [Toolkit Cleanup] Yard doc: step 1 – config cleanup Feb 1, 2021

AliSoftware commented Feb 1, 2021

View reviewed changes

jkmassel approved these changes Feb 1, 2021

View reviewed changes

mokagio approved these changes Feb 2, 2021

View reviewed changes

AliSoftware mentioned this pull request Feb 2, 2021

Fix "alpha" naming convention #202

Open

AliSoftware added 4 commits February 2, 2021 17:17

Fix typos/formatting

3b280f9

yardopts CRLF at EOF

2bc0401

abort if PROJECT_ROOT_FOLDER undefined

0813fe6

Instead of guessing a possibly wrong default value

Remove constants @group, not really useful

263ab3d

AliSoftware merged commit 5d8271a into toolkit-cleanup Feb 2, 2021

AliSoftware deleted the yard-doc/step1 branch February 2, 2021 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Toolkit Cleanup] Yard doc: step 1 – config cleanup #198

[Toolkit Cleanup] Yard doc: step 1 – config cleanup #198

AliSoftware commented Feb 1, 2021 •

edited

Loading

AliSoftware Feb 1, 2021

AliSoftware Feb 1, 2021

AliSoftware Feb 1, 2021

AliSoftware Feb 1, 2021

jkmassel left a comment •

edited

Loading

jkmassel Feb 1, 2021

jkmassel Feb 1, 2021

jkmassel Feb 1, 2021

AliSoftware Feb 1, 2021 •

edited

Loading

AliSoftware Feb 2, 2021

jkmassel Feb 1, 2021

AliSoftware Feb 2, 2021

jkmassel Feb 1, 2021

AliSoftware Feb 2, 2021 •

edited

Loading

jkmassel Feb 1, 2021

jkmassel Feb 1, 2021

jkmassel Feb 1, 2021

mokagio left a comment

mokagio Feb 2, 2021

mokagio Feb 2, 2021

mokagio Feb 2, 2021

AliSoftware Feb 2, 2021

		module AndroidVersionHelper
		# The key used in internal version Hash objects to hold the versionName value

		module IosVersionHelper
		# @!group The indices for various parts of the version parts array.

[Toolkit Cleanup] Yard doc: step 1 – config cleanup #198

[Toolkit Cleanup] Yard doc: step 1 – config cleanup #198

Conversation

AliSoftware commented Feb 1, 2021 • edited Loading

To Test

Proof-reading the documentation

Double-checking the methods that have been renamed

Notes

Incremental PRs and WIP

Confusing names and refactoring

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkmassel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Feb 1, 2021 •

edited

Loading

jkmassel left a comment •

edited

Loading

AliSoftware Feb 1, 2021 •

edited

Loading

AliSoftware Feb 2, 2021 •

edited

Loading