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

FAKE template domain specific language choice #2177

Merged

Conversation

florianbader
Copy link
Contributor

Description

Adds an additional choice to the FAKE template to specify the used domain specific language. At the moment this is either the default FAKE DSL or the BlackFox DSL.

@matthid
Copy link
Member

matthid commented Oct 28, 2018

hehe :)
/fyi @vbfox @kblohm

@vbfox
Copy link
Contributor

vbfox commented Oct 29, 2018

Nice 😉

I would prefer buildtask as a name as I don't think anyone really care about my pseudo (The name is pretty generic but I'm never really inspired with names)

@matthid
Copy link
Member

matthid commented Oct 29, 2018

@vbfox I guess you mean --dsl buildtask?

@matthid
Copy link
Member

matthid commented Oct 29, 2018

Only thing is that we could add a test for this flag (we already have some tests for the template so it should be reasonably easy to add another one), let me know if you need help.

@matthid matthid added the needs-tests This PR needs tests in order to be accepted label Oct 29, 2018
@vbfox
Copy link
Contributor

vbfox commented Oct 29, 2018

@matthid yes 😄

@florianbader
Copy link
Contributor Author

florianbader commented Oct 29, 2018

Nice 😉

I would prefer buildtask as a name as I don't think anyone really care about my pseudo (The name is pretty generic but I'm never really inspired with names)

@vbfox I think buildtask is a bit too generic, but I also can't find a better name. Maybe blackfox might not be that bad, because it gives credit to the author ;)

@kblohm
Copy link
Contributor

kblohm commented Oct 29, 2018

The names "proposed" in the original issue were "pipelines" and "target groups". I would change it to buildtask though just because the api is named that way.

@matthid
Copy link
Member

matthid commented Oct 29, 2018

Maybe blackfox might not be that bad, because it gives credit to the author ;)

We will see how it works it's more or less an experiment in how external modules might work (and if we can move more outside this repository) we will see how it works. We already have seen issues regarding issues with the level of binary compatibility we provide. So we might come to the conclusion that external modules are not too viable and prefer to have the code here (which I don't hope).
Anyway just something to consider

I'm pretty sure if we can come up with a good name we can still change it including the module name (if @vbfox agrees) but we shouldn't change it every version. It is indeed a bit strange to call "targets" "tasks", or is it?

@matthid
Copy link
Member

matthid commented Oct 29, 2018

Also It's not only "build" but also deploy and every other kind of automation ;)

Adds link to dsl documentation in template help
Adds template tests for dsl and dependencies choices
@florianbader
Copy link
Contributor Author

@kblohm Fixed your comments.
@matthid Added tests to the best of my ability. They work but I'm open for hints on how to improve my F# skills 😄

@vbfox
Copy link
Contributor

vbfox commented Oct 29, 2018

We already have seen issues regarding issues with the level of binary compatibility we provide

I think it can be done especially with FAKE 5, the central API surface is quite small and easier to version. I hope it lead to FAKE packages being versionned separately with a highly compatible core and packages around it that experiment more freely.

Some things like the Record compatibility problems could even be suggested as enhancements to the F# language as it's an especially annoying point of F# APIs

I'm pretty sure if we can come up with a good name we can still change it including the module name

No problem changing it but it already changed a few times :

  • The first versions used target (lowercase) as there was no strong typing, only a different declaration syntax for Target dependencies
  • When typed results were added it was TypedTarget then Task because it's how gulp name them ( https://gulpjs.com/docs/en/api/concepts#tasks )
  • But it colision with .Net Task and with the task CE it's not impossible to use both so when I finally decided to make a NuGet I choose BuildTask as the most unoriginal thing that removed the ambiguity

@matthid
Copy link
Member

matthid commented Oct 29, 2018

Some things like the Record compatibility problems could even be suggested as enhancements to the F# language as it's an especially annoying point of F# APIs

Indeed that's what I was thinking about. This is really unfortunate and not something we are able to handle at the moment

Sadly, it's the most used feature throughout all the modules, maybe with the exception of the target module. Maybe the record stuff can be solved but there are similar limitations with DUs and I don't think they are solvable by F#. In fact I doubt a lot of people can write code which we would consider 'forward compatible' ;)
But maybe time will tell us that what we do works in practice.

Regarding naming, have you tried calling your module 'Target' and hiding existing members (people can still access stuff via fully qualified names?)

@matthid matthid removed the needs-tests This PR needs tests in order to be accepted label Nov 1, 2018
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.

@vbfox Maybe like this?

Specifies your prefered way to define build tasks inside your build script:

- `fake` (default) - Uses the default FAKE domain specific language
- `blackfox` - Uses the BlackFox domain specific language
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `blackfox` - Uses the BlackFox domain specific language
- `buildtask` - Uses a string free domain specific language, called [BuildTask](https:/vbfox/FoxSharp/blob/master/src/BlackFox.Fake.BuildTask/Readme.md)

"description": "Uses the default FAKE domain specific language (see https://fake.build/core-targets.html)"
},
{
"choice": "blackfox",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"choice": "blackfox",
"choice": "buildtask",

},
{
"choice": "blackfox",
"description": "Uses the BlackFox domain specific language (see https:/vbfox/FoxSharp/blob/master/src/BlackFox.Fake.BuildTask/Readme.md)"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": "Uses the BlackFox domain specific language (see https:/vbfox/FoxSharp/blob/master/src/BlackFox.Fake.BuildTask/Readme.md)"
"description": "Uses a string free domain specific language, called [BuildTask](https:/vbfox/FoxSharp/blob/master/src/BlackFox.Fake.BuildTask/Readme.md)"

nuget Fake.Core.Target
//#if (dsl == "blackfox")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//#if (dsl == "blackfox")
//#if (dsl == "buildtask")

type DslKind =
| Fake
| BlackFox
with override x.ToString () = match x with | Fake -> "fake" | BlackFox -> "blackfox"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with override x.ToString () = match x with | Fake -> "fake" | BlackFox -> "blackfox"
with override x.ToString () = match x with | Fake -> "fake" | BlackFox -> "buildtask"

@vbfox
Copy link
Contributor

vbfox commented Nov 2, 2018

Regarding naming, have you tried calling your module 'Target' and hiding existing members (people can still access stuff via fully qualified names?)

I'm not a fan of hiding stuff or depending on open ordering, It's easy to miss when copy pasting code or reading someone else and make editors auto-open feature a lot less useful

@kblohm
Copy link
Contributor

kblohm commented Nov 2, 2018

Just of the top of my head, for discussion purposes:

  • TargetGroup
  • TargetPipeline
  • TargetDsl
  • TargetEx (The classic)
  • TargetBuilder
  • TargetCe
  • FakeTarget
  • Task (Cake/Rake uses Task)
  • FakeTask
    I would prefer something with target though, that does sound more familiar in FAKE.
    Or you could maybe just expand the Target-Module with Target.createGroup/Target.createBuilder etc.? So you dont have to hide existing ones. Or Target.Group.create/Target.FromGroup.create/Target.Builder.create, similair to Target.WithContext?

@matthid
Copy link
Member

matthid commented Nov 2, 2018

Afaics the implementation is not particularly related to pipelines or groups it's more accidental that it solves the same problem in a nice way. But in the end you define classical "targets" and their dependencies under the hood. You can even mix the two.

However, I feel like just calling them "Tasks" and marketing them as the new way of doing this would be fine by me as well.

Even after @vbfox I still like just 'Target' and 'TargetEx' or 'Task' from @kblohm. Additionally I'd suggest 'StrongTarget' or 'TypedTarget', 'QuickTarget' or 'BTarget' (better target or blackfox target, beta target for being second)

Yes we could also not hide but only extend the Target module, like 'createTyped' instead of just 'create' for example.

@matthid
Copy link
Member

matthid commented Nov 2, 2018

In the end I'd like to get something out and not wait too long on naming, as the module is external we can change naming, add a new module and not break anyone

@florianbader
Copy link
Contributor Author

I like the idea of calling it task instead of buildtask. I'm updating the PR later so we might be able to close it for now.

@vbfox
Copy link
Contributor

vbfox commented Nov 2, 2018

I like task, and would have kept it as a name if it didn't collision with a well known framework type and computation expression

@matthid matthid self-assigned this Nov 6, 2018
@matthid matthid merged commit 7cf5070 into fsprojects:release/next Nov 10, 2018
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.

4 participants