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

Don't set environment in resetEditorFrom() #739

Merged
merged 2 commits into from
Jun 17, 2016
Merged

Don't set environment in resetEditorFrom() #739

merged 2 commits into from
Jun 17, 2016

Conversation

etanshaul
Copy link
Contributor

fixes #738

Since we are explicitly passing in the environment to the Editor constructor, there is no need to reset it from the configuration.

@etanshaul
Copy link
Contributor Author

@patflynn or @chanseokoh

@patflynn
Copy link
Contributor

LGTM.

Remind me why we're storing it in the configuration again?

@etanshaul
Copy link
Contributor Author

Sure - that was for deserialization - see #734

@patflynn
Copy link
Contributor

oh right. You can't determine the environment at deserialization time. We should add a comment explaining why it's there.

@chanseokoh
Copy link
Contributor

How can configuration.getEnvironment() be null in the first place? Does this happen after an IDE restart (after deserialization)?

@etanshaul
Copy link
Contributor Author

@chanseokoh When you create a new deployment config for the first time, resetEditorFrom() is invoked with a configuration who's values are all null.

@etanshaul
Copy link
Contributor Author

@patflynn I added a comment explanation on the member variable in the deployment configuration.

@patflynn
Copy link
Contributor

Thanks!

On Fri, Jun 17, 2016 at 4:20 PM, Etan Shaul notifications@github.com
wrote:

@patflynn https://github.com/patflynn I added a comment explanation on
the member variable in the deployment configuration.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#739 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHf5HZO5vbIRZ8ck1J0jE3eA_0SFySryks5qMwF7gaJpZM4I4q9N
.

@etanshaul
Copy link
Contributor Author

@chanseokoh let me know if you are ok with this. I admit I was surprised by the behavior myself.

@chanseokoh
Copy link
Contributor

🚢

@etanshaul etanshaul merged commit 9f3e9c1 into master Jun 17, 2016
@etanshaul etanshaul deleted the env-npe branch June 17, 2016 20:34
aslo pushed a commit that referenced this pull request Jun 28, 2016
…ij into upgrade-ij-gradleplugin

* 'master' of github.com:GoogleCloudPlatform/gcloud-intellij: (30 commits)
  IJ build version (#762)
  Update tracking api clients (#760)
  Update CHANGELOG.md (#759)
  Experimenting with new tracking API (#752)
  Release v0.9.6 beta (#758)
  Service lookup should use interface (#757)
  Release v0.9.5 beta (#756)
  Identify and properly label all compat projects (#753)
  Follow the convention /virtual/eventType/eventName (#754)
  Updates our app-tools-lib-for-java to the new name and released version. (#755)
  Retrieve plugin name from PluginInfoService (#743)
  Track deploy and stop task execution events (#750)
  Create pre-build task for app engine artifact sources (#749)
  Update changelog release date (#747)
  Restructure metrics event actions (#744)
  Don't show custom deployment options when flex compat (#745)
  Don't set environment in resetEditorFrom() (#739)
  Sync up with app-tools - remove setting of gsUtil (#737)
  Update changelog for next release (#735)
  Save and load artifact deployment source types (#734)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE in deployment config editor due to environment reseting
4 participants