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

2.15.2 #94

Merged
merged 10 commits into from
Oct 9, 2018
Merged

2.15.2 #94

merged 10 commits into from
Oct 9, 2018

Conversation

dwrusse
Copy link
Contributor

@dwrusse dwrusse commented Oct 8, 2018

No description provided.

@scrthq scrthq self-requested a review October 9, 2018 02:15
@scrthq
Copy link
Member

scrthq commented Oct 9, 2018

Thanks for sending over the PR, @dwrusse!! Would you mind if I made some updates to this? Here's what I have in mind so that things flow with the rest of the functions:

  • Change function name to Update-GSGmailLabel since it's using an Update() method
    • This could really go either way, but the general use within this module
  • Adjust parameter casing where needed from camelCase to PascalCase
  • Move $SourceObject within the function's blocks or let it purely accept pipeline input of Label objects
    • This may be useful for another parameter set when piping in the output of Get-GSGmailLabel, but having another function as the default can get a bit messy
  • Adjust function to use the Patch() method instead of Update()
    • Since the end goal (IMO) would be to update just the portions of the Label that you need, we can use this to eliminate an API call to get the Label object before we update it by using Patch() to only target the segments we want to update, then build our $body properties dynamically based on that.

I don't want to step on your toes and greatly appreciate you sending this over!! Let me know your feedback on it and let's use this to open a discussion up on things 😄

@scrthq scrthq self-assigned this Oct 9, 2018
@dwrusse
Copy link
Contributor Author

dwrusse commented Oct 9, 2018

Thanks for reviewing @scrthq . All of your suggestions make sense and I agree with your reasoning, so no objections here. I can make the updates myself tomorrow if you'd like.

I've also added the Set-GSGmailMessageLabel function if you'd like to include it in this version.

@scrthq
Copy link
Member

scrthq commented Oct 9, 2018

Thanks @dwrusse! I'm actually almost done with the updates on my end, working off your forked branch now (this PR was actually good timing since I was just hopping on my dev env 😄). Wrapping in Color update support as well for full coverage of the available update fields

Thanks for adding in the Set-GSGmailMessageLabel function as well!! I'll check it out next! 😄

@ghost ghost added the work in progress label Oct 9, 2018
Copy link
Member

@scrthq scrthq left a comment

Choose a reason for hiding this comment

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

Thanks, @dwrusse!! Updates done, getting this merged ASAP! Cheers! 🎉

@scrthq scrthq merged commit 7248bc0 into SCRT-HQ:master Oct 9, 2018
@ghost ghost removed the work in progress label Oct 9, 2018
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.

2 participants