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

Add support for scheme environment variables #326

Conversation

justinseanmartin
Copy link
Contributor

This resolves issue #325.

Aside from the stuff directly related to environment variables, I did some rubocop cleanup. After rebasing, the changes are mostly moot due to other changes to the .rubocop_cocoapods.yml. I also reran rubocop --auto-gen-config, which looks like it hasn't been run in a while. I can break these changes out to a separate PR, or remove them entirely if you'd prefer. LMK.

🌈 (after the fact)

@justinseanmartin justinseanmartin force-pushed the jmartin/add-environment-variable-support branch 2 times, most recently from d39abc2 to 208246b Compare October 26, 2015 00:18
@AliSoftware AliSoftware self-assigned this Oct 26, 2015
# LaunchAction or TestAction scheme group.
#
class EnvironmentVariables < XMLElementWrapper
# @param [REXML::Element,Array{EnvironmentVariable},Array{Hash{String => String}}] node_or_target
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the YARD documentation syntax expect Array<EnvironmentVariable>, see http://yardoc.org/types.html for a quick cheat sheet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you're right. First time for YARD, read through the documentation. Will fix this up all over.

@AliSoftware
Copy link
Contributor

Thanks for the PR, looks very promising!

Ok for me once my remarks have all been addressed.


# @return [Hash{:key => String, :value => String, :enabled => Boolean}]
# The environment variable XML node with attributes converted to Hash keys
def to_h
Copy link
Contributor

Choose a reason for hiding this comment

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

The ruby standard name for this method is to_hash, not to_h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be differing information. From what I've read, appears that to_hash is specifically if the object acts like a hash and can be converted with no loss of fidelity. I don't think that is the case here. I don't feel strongly though, so LMK if I'm just misunderstood.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, actually you're right, didn't know about that. Let's keep to_h then.

# @return [Array<EnvironmentVariable>]
# The new set of environment variables after addition
#
def assign_variable(variable)
Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely prefer that assign_variable.

BTW, talking about [] and []= it actually seem like a good addition (but still while keeping this assign_variable of course).

While assign_variable would take an EnvironmentVariable, Hash{Symbol=>String,Bool}, []= would take the variable name as key and expect the variable content as value (and would assume enabled=YES, no way to specify enabled with that syntax). And [] would take the variable name as key and return the EnvironmentVariable.
That could provide a nice syntactic sugar alternative. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me. Should [] operate on enabled variables only as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so, since it would return an EnvironmentVariable anyway, I'd say let it access all variables (enabled or disabled). The user could still check the variable state once retrieved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@AliSoftware
Copy link
Contributor

Sorry not to have spotted that before, but could you fix the alignment/identation of all your YARD docs?

Convention is to align the text associated with @param so that it starts at the same column as the param type, like this

# Some description
#
# @param [Type] paramName
#        This parameter description starts as the same indentation level as the type
#        and continues with the same alignment if it wraps on multiple lines
#
def foo(paramName)
  ...
end

After that, I think I'm good 👍

@justinseanmartin justinseanmartin force-pushed the jmartin/add-environment-variable-support branch from 1a11ee9 to 9ce8dc2 Compare October 27, 2015 11:13
@justinseanmartin
Copy link
Contributor Author

All feedback addressed and squashed the commits.

# Assigns a value for a specified key
#
# @param [String] key
# The key to update in the environment variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Still misaligned ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@justinseanmartin justinseanmartin force-pushed the jmartin/add-environment-variable-support branch from 9ce8dc2 to 14e13f6 Compare October 27, 2015 11:30
@AliSoftware
Copy link
Contributor

LGTM now 👍 Will merge in a few once Travis is OK.

Nice work @justinseanmartin 🎉

@justinseanmartin
Copy link
Contributor Author

Thanks for the prompt review feedback and dealing with a first time project contributor 😃

@AliSoftware
Copy link
Contributor

Then even more awesome work for a first-time contributor then! 👍

AliSoftware added a commit that referenced this pull request Oct 27, 2015
…-variable-support

Add support for scheme environment variables
@AliSoftware AliSoftware merged commit 79b3f4d into CocoaPods:master Oct 27, 2015
@tirodkar
Copy link

I don't think this is pushed out to the latest release yet. This would be a great enhancement to have. Is there an ETA for the next release?

@segiddins
Copy link
Member

@tirodkar there's no fixed release schedule, but this will ship with 1.0

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.

4 participants