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

Fix Environment retrieve methods #2653

Merged
merged 5 commits into from Mar 16, 2021

Conversation

DimaMishchenko
Copy link
Contributor

@DimaMishchenko DimaMishchenko commented Mar 12, 2021

Description πŸ“

Fix of Environment variable retrieve methods provided in #1468

Currently this functionality works incorrect.
If variable is not exists, dynamicMember will return nil and getString/getBoolean will never be called.
Also in case if variable exist and you use helper methods you still will get optional value.
Can be fixed by creating extension to Optional type.

Example

// appName not exists

// before
Environment.appName?.getString(default: "TuistApp") // nil

// now
Environment.appName.getString(default: "TuistApp") // "TuistApp"

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Hi @DimaMishchenko! Changes look good, as mentioned, it'd be great to make this change non-breaking

Sources/ProjectDescription/Environment.swift Show resolved Hide resolved
Added backward compatibility to Environment retrieve methods.
Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Looks good to me - I'd just change the deprecation message but otherwise πŸ‘ from me

Sources/ProjectDescription/Environment.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Good catch @DimaMishchenko - thanks for fixing this πŸ‘

One additional suggestion would be to perhaps update the fixture https://github.com/tuist/tuist/blob/main/fixtures/framework_with_environment_variables/Project.swift to leverage the new syntax.

* main:
  Rebuild frameworks
  Remove unnecessary step
  Add Luis to the core team
  Fix Xcode version
  Fix syntax issue
  Bump gatsby-plugin-mdx from 1.10.0 to 2.0.1 in /next (tuist#2652)
  Plugins: Finalize support for tuist edit (tuist#2642)
  Drop support for Xcode 11.x (tuist#2651)
  docs: add tiarnann as a contributor (tuist#2657)
  Install script bug fix: Adding bin folder to usr/local/ when it is missing (tuist#2655)

# Conflicts:
#	CHANGELOG.md
@DimaMishchenko
Copy link
Contributor Author

Done. Thank you @fortmarek and @kwridan

@fortmarek
Copy link
Member

let's merge this once CI is green, unfortunately, we have some issues on main. We should re-run the tests here once that is fixed.

@pepicrft
Copy link
Contributor

@all-contributors add @DimaMishchenko for code

@allcontributors
Copy link
Contributor

@pepibumur

@DimaMishchenko already contributed before to code

@fortmarek
Copy link
Member

@DimaMishchenko could you please rebase and see if the pipeline succeeds now? The main should be fixed now. Sorry for the additional troubles πŸ™‚

@pepicrft pepicrft merged commit 15a5021 into tuist:main Mar 16, 2021
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.

None yet

4 participants