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.

Sign up for GitHub

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

Jump to bottom

Migrate Fake.Api.Slack to NET Core #1648

Merged
merged 7 commits into from
Sep 18, 2017
Merged

Migrate Fake.Api.Slack to NET Core #1648

merged 7 commits into from
Sep 18, 2017

Conversation

vlukash
Copy link
Contributor

@vlukash vlukash commented Sep 1, 2017 •
edited
Loading

Migrate Fake.Api.Slack to NET Core

Files:

SlackNotificationHelper.fs

New namespace:

Fake.Api

New module name:

Slack

New *.fsx script example:

open Fake.Core
open Fake.Api

Target.Create "SendMsgToSlack" (fun _  ->
    let webhookUrl = "https://hooks.slack.com/services/some/random/text"
    Slack.SendNotification webhookUrl (fun p ->
        {p with
            Text = "My Slack Notification!\n<https://google.com|Click Here>!"
            Channel = "#general"
            Icon_Emoji = ":ghost:"
            Attachments = [| 
                {Slack.NotificationAttachmentDefaults with
                    Fallback = "Attachment Plain"
                    Text = "Attachment Rich"
                    Pretext = "Attachment Pretext"
                    Color = "danger"
                    Fields = [|
                        {Slack.NotificationAttachmentFieldDefaults with
                            Title = "Field Title 1"
                            Value = "Field Value 2"}
                        {Slack.NotificationAttachmentFieldDefaults with
                            Title = "Field Title 1"
                            Value = "Field Value 2"}|]
                }
                {Slack.NotificationAttachmentDefaults with
                    Fallback = "Attachment 2 Plain"
                    Text = "Attachment 2 Rich"
                    Pretext = "Attachment 2 Pretext"
                    Color = "#FFCCDD"
                }|]
        })
    |> printfn "Result: %s"
)

Target.RunOrDefault "SendMsgToSlack"

Tested on:

  • Ubuntu 16.04, mono 5.2.0.215
  • Ubuntu 16.04, NET Core 2.0

@vlukash vlukash changed the title [WIP] Migrate Fake.Api.Slack to NET Core Migrate Fake.Api.Slack to NET Core Sep 1, 2017
@matthid
Copy link
Member

matthid commented Sep 4, 2017

Thanks for this it looks really nice!
Sorry for the delay. There were some problems.

Currently master is "green" (there is still a transient error from time to time). Please try to rebase/merge to get this green.

About the namespace: Fake.Api is probably fine. I'm currently thinking about where to add Octokit (Github API). But Fake.Api.Github might just be the right place. I'm just a bit afraid that "Api" might be too general? What do you think?

@vlukash vlukash force-pushed the vlukash/slack_netcore branch from 9386a05 to 4685cc3 Compare September 4, 2017 18:40
@vlukash
Copy link
Contributor Author

vlukash commented Sep 4, 2017

@matthid thanks for fixing the build!

About the Fake.Api namespace. Yes, probably it's a good idea to create more specific namespaces. What about a Fake.Integrations for the Slack?
Please let me know your options/preference for the namespace so I can update it if needed.

matthid
matthid approved these changes Sep 17, 2017
View reviewed changes
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Thanks, sorry for the late review. I left some comments but leaving the namespace called Fake.Api is probably fine.

src/app/Fake.Api.Slack/SlackNotification.fs Outdated
/// Contains a task to send notification messages to a [Slack](https://slack.com/) webhook
module Slack =
/// The Slack notification attachment field parameter type
[<CLIMutable>]
Copy link
Member

Choose a reason for hiding this comment

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

I kind of removed [<CLIMutable>] when porting records - it had historical reasons, but was not actually used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, removed.

src/app/Fake.Api.Slack/SlackNotification.fs Outdated
}

/// The Slack notification attachment parameter type
[<CLIMutable>]
Copy link
Member

Choose a reason for hiding this comment

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

same here

src/app/Fake.Api.Slack/SlackNotification.fs Outdated
/// The title of the attachment
Title: string
/// Content to which the title should link
Title_Link: string
Copy link
Member

Choose a reason for hiding this comment

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

hm should we really use _ here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

src/app/Fake.Api.Slack/SlackNotification.fs Outdated
}

/// The Slack notification parameter type
[<CLIMutable>]
Copy link
Member

Choose a reason for hiding this comment

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

remove

src/app/Fake.Api.Slack/SlackNotification.fs Outdated
/// - `setParams` - Function used to override the default notification parameters
let SendNotification (webhookURL : string) (setParams: NotificationParams -> NotificationParams) =
let sendNotification param =
#if NETSTANDARD
Copy link
Member

Choose a reason for hiding this comment

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

please don't indent compiler directives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@vlukash
Copy link
Contributor Author

vlukash commented Sep 18, 2017 •
edited
Loading

@matthid thanks for the review.
Removed CLIMutable and _.

@matthid matthid merged commit cd3d612 into fsprojects:master Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vlukash @matthid