Skip to content

Add named params and yard docs to create_milestone method at GithubHelper #426

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Nov 17, 2022
Merged

Add named params and yard docs to create_milestone method at GithubHelper #426

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
66ccfcf
GithubHelperSpec: Add Yard Docs and Named Parameters to create_milest…
raafaelima Nov 6, 2022
b658f87
CreateNewMilestoneAction: Adds named parameters to create_milestone G…
raafaelima Nov 6, 2022
10528de
GithubHelper#create_milestone: Rephrase yard docs
raafaelima Nov 6, 2022
255d108
GithubHelper#create_milestone: Rename parameters to be more descriptive
raafaelima Nov 7, 2022
99a95ac
CreateNewMilestoneAction: Rephrase comment about release times to be …
raafaelima Nov 8, 2022
39b23c8
GithubHelper#create_milestone: Add unit tests and validation to date …
raafaelima Nov 8, 2022
0750081
Merge branch 'trunk' into add/named-params-create-milestone
raafaelima Nov 14, 2022
b218ccc
GithubHelper: Fix Timezone changes unit tests
raafaelima Nov 14, 2022
de90d4d
GithubHelper: Adds more unit tests to validate the Timezone changes
raafaelima Nov 14, 2022
d87c03c
GithubHelper: Fix Rubocop Issues
raafaelima Nov 16, 2022
7f99d51
GithubHelperSpec: Adds comments to DST cases to be more clear and new…
raafaelima Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_new_milestone_action.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,14 @@ def self.run(params)
newmilestone_number = Fastlane::Helper::Ios::VersionHelper.calc_next_release_version(last_stone[:title])
number_of_days_from_code_freeze_to_release = params[:number_of_days_from_code_freeze_to_release]
UI.message("Next milestone: #{newmilestone_number} due on #{newmilestone_duedate}.")
github_helper.create_milestone(repository, newmilestone_number, newmilestone_duedate, milestone_duration, number_of_days_from_code_freeze_to_release, params[:need_appstore_submission])
github_helper.create_milestone(
repository: repository,
newmilestone_number: newmilestone_number,
newmilestone_duedate: newmilestone_duedate,
newmilestone_duration: milestone_duration,
number_of_days_from_code_freeze_to_release: number_of_days_from_code_freeze_to_release,
need_submission: params[:need_appstore_submission]
)
end

def self.description
Expand Down
13 changes: 12 additions & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/helper/github_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,18 @@ def get_last_milestone(repository)
last_stone
end

def create_milestone(repository, newmilestone_number, newmilestone_duedate, newmilestone_duration, number_of_days_from_code_freeze_to_release, need_submission)
# Creates a new milestone
#
# @param [String] repository The repository name, including the organization (e.g. `wordpress-mobile/wordpress-ios`)
# @param [String] newmilestone_number The name of the milestone we want to create (e.g.: `16.9`)
# @param [Time] newmilestone_duedate milestone due date (e.g. `2022-10-22T12:00:00Z`)
# @param [Integer] newmilestone_duration Number of days that a milestone extents
# @param [Integer] number_of_days_from_code_freeze_to_release Number of days from code freeze to release
# @param [Boolean] need_submission The app needs to be submitted?
# if `true`, will subtract 3 days from the `:number_of_days_from_code_freeze_to_release`.
# if `false`, will use the days of `:newmilestone_duration`
#
def create_milestone(repository:, newmilestone_number:, newmilestone_duedate:, newmilestone_duration:, number_of_days_from_code_freeze_to_release:, need_submission:)
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter names seems a bit of a mouthful to me, given we already know from the context (of them being parameters of create_milestone) that those are about new milestones.

For example, what about naming the parameters just title, due_date, duration, and days_until_release?

Though a probably even nicer solution, especially because duration and need_submission are only used once and are kind of exclusive (i.e. either we pass need_submission: false and then duration doesn't matter, or we pass need_submission: true and that's only when duration is used…), we should probably combine the goal of the two parameters into a single one there, which would be days_until_submission, and let the caller pass to this value.

Given that, I suggest the following new API for this helper method:

      def create_milestone(repository:, title:, due_date:, days_until_submission:, days_until_release:)

Then:

  • For the time being, the call site in create_new_milestone_action would just do the computation on its end, i.e. pass params[:need_appstore_submission] ? number_of_days_from_code_freeze_to_release - 3 : milestone_duration for the new days_until_submission parameter of this GithubHelper#create_milestone, carrying over the "Using 3 days mostly for historical reasons" logic from lines 88-89 for now
  • In the future, when we will consider updating the API of the create_new_milestone action itself one day, we would then be able to update its ConfigItem (in a breaking-change way) to have the callers also directly provide the days_until_submission and days_until_release values in their Fastfile's call sites instead of duration and/or need_submission

But for now, that change would make for a nicer and easier to read and understand API for the internal GithubHelper#create_milestone, all while preparing for the future of the planned refactor of the corresponding action and public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, those parameters that are in place are big!
I'll address those changes, thanks for the input 😄

# If there is a review process, we want to submit the binary 3 days before its release
#
# Using 3 days is mostly for historical reasons where we release the apps on Monday and submit them on Friday.
Expand Down
12 changes: 6 additions & 6 deletions spec/github_helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,12 @@ def get_milestone(milestone_name:)
def create_milestone(need_submission:, milestone_duration:, days_code_freeze:)
helper = described_class.new(github_token: 'Fake-GitHubToken-123')
helper.create_milestone(
test_repo,
test_milestone_number,
test_milestone_duedate.to_time.utc,
milestone_duration,
days_code_freeze,
need_submission
repository: test_repo,
newmilestone_number: test_milestone_number,
newmilestone_duedate: test_milestone_duedate.to_time.utc,
newmilestone_duration: milestone_duration,
number_of_days_from_code_freeze_to_release: days_code_freeze,
need_submission: need_submission
)
end
end
Expand Down