Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[spaceauth] add
--copy_to_clipboard
option tospaceauth
#18538[spaceauth] add
--copy_to_clipboard
option tospaceauth
#18538Changes from all commits
f5d9930
ae54760
42c0dc0
4b14cef
aef508d
b2b214f
07acc41
bbf9c33
203c687
20f3d8a
88ce490
829064f
0934348
cf68c18
7ded3b6
6572f48
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like reorganising dependencies alphabetically, but it may have some unintended side-effects in Ruby.
This PR works on my machine, but let's wait for CircleCI's unit tests to complete before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the tests pass, is this safe? I have no clue 😮 what are/were the risks? maybe cc @ainame 🙏 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CircleCI passed, so it should be safe ✅
As you saw in fastlane_core.rb, some requires have to be ordered before:
And some after:
They are usually extensions of Ruby classes, without those we'll probably get
NoMethodError
s in CircleCI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as test cases cover those files, it would be fine🙂 Even in the monkey patch scenario mentioned, it doesn't matter as long as Ruby doesn't evaluate such patched methods directly.
For example, let's say we need to load two files in
fastlane_core.rb
andfile A
is like below, it is safe. The order doesn't matter.monkey_patch_method_return_true
will be evaluated after everything loaded.However,
file A'
causes an error because we need torequire
file 'moneky_patch' that implementsmonkey_patch_method_return_true
to String beforefile A
is loaded.commander
scenario surely depends on the gem to define the classes first and then monkey patching follows.You can test whether that file loading works by
ruby -Ifastlane_core/lib fasltane_core/lib/fastlane_core.rb
. Even so, there is a small chance order updates create logic-level discrepancy but unit tests should cover that 🤞It's off-topic but
fastlane
's unit testing runs test cases exactly the same order every time right now so combining existingrequire
order and unit test casesorder
may create a happy code path that still leaves some edge-cases technically😬I've been trying to improve that in a PR...
#18278
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This slightly terrifies me but I do prefer it this way 😬 I don’t think this should causes any issues but still a bit spooky 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would change the default behaviour...
See #18538 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ Yeah, here the absence of the param behaves differently than an explicit
false
value, hence why the check up there is== false
instead of a'falsey'
checkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for that, it simplified my logic to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I just noticed the
session_string
method.So the proper way is:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just return
@yaml
? 😇 This is already running on an instance so I don’t think returningself
is as useful as returning the actual value? 🤔This also gets rid of the need for the
session_string
addition below 🤷♂️cc: @rogerluan @return-main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be okay with that, but by returning
self
we can future-proof the lane in case we create new methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share the same thoughts as @return-main ☝️
Not sure how/if this class plans to be changed much in the future, but it's a possibility. It would make it possible to create new methods similar to how
session_string
works, without worrying about retrocompatibility.What do you think @joshdholtz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine with that 😇 We should be good to go then