Skip to content

Conversation

@Krzysztof-Cieslak
Copy link

@Krzysztof-Cieslak Krzysztof-Cieslak commented Apr 30, 2020

This is more a proof of concept and a start of discussion rather than complete implementation. So... as in title - it extends current dotnet CLI with dotnet add file.

This feature is probably mostly useful for F# developers - in C# users can use globbing, but in F# order of files in project matters - so we specify every .fs file separately in the project file. Having this feature will be also great for better editor integration - VSCode (with Ionide) is used by a huge part of the F# community and we provide "solution explorer" view in the editor but we need good commands to add files and/or move the up-down (again the order of files matters) - we currently provide that functionality using a custom utility (https://github.com/ionide/Forge) but... well, let's just say it's not my best work ever.

So there's a couple of things to discuss here:

  • would you be open to having such a feature?
  • how to determine default location of the file (probably on top of the group, current implementation adds it adds the bottom)
  • should it have something like --above and --below options that would allow specifying a location of the file (it's a common scenario to say "I want to add helpers.fs above my_test.fs")
  • should it allow not only adding but also moving files (changing the order of files and moving files up/down) is another common scenario during the development of F# projects

CC: @cartermp

P.S. the changes include all generated translation files, please let me know if they shouldn't be included in PR - there's no contribution guide in the repo that would specify such details ;-)

P.S.2

Even tho, it's proof of concept it actually works ✨

image

@cartermp cartermp requested a review from wli3 May 2, 2020 02:22
@Krzysztof-Cieslak
Copy link
Author

🛎️

@wli3
Copy link

wli3 commented May 19, 2020

Sorry I missed the notification.

@wli3
Copy link

wli3 commented May 19, 2020

@KathleenDollard what do you think about this PR

@wli3
Copy link

wli3 commented May 19, 2020

Thank you for the contribution!

My concerns:

  1. I don't know how useful this command is. I've written several F# app. But add file is one thing. But the most important thing is to order the files. And adding a files is very straight forward with editing the project file.

  2. I don't know if it will confuse C# developers to add file when they don't need to

  3. no test for this PR. But this could come later once we decide we want this feature (and we should write unit test instead of dotnet add file level test)

@KathleenDollard
Copy link

I like the idea, but want to understand the impact and work out the full syntax. This might be better done in an issue, if you open one and reference it I'll move further comments there. (Separating the idea from this specific PR).

@Krzysztof-Cieslak could you mock up the help (use dotnet add package as the template) so anyone reviewing can understand the full syntax. Include tentative descriptions and we can fix them later.

@Krzysztof-Cieslak, @cartermp and other F# folks could you comment on the value of this in F#

  • Should the default should be before or after?
  • If we have an add, do we need a remove?
  • It actually does an include. Will it be confusing to call include add? What do we mimic, the project file or other dotnet add features?

Everyone:

  • Will this trip you up if you are a C# developer? Can we fix that in the description?
  • Would an exclude make sense?
  • Other thoughts.

@Krzysztof-Cieslak
Copy link
Author

I don't know how useful this command is. I've written several F# app. But add file is one thing. But the most important thing is to order the files. And adding files is very straight forward with editing the project file.

Sure, as I've mentioned above - actually interesting things are:

  • adding a file in a particular position (above or below some other given file)
  • moving files around.

But I've implemented just simplest thing to see how difficult it would be, and just use it as a starter for discussion. It would be probably not worth to add only what is currently in this PR.

no test for this PR

What are tests? 🙃 (again, it's just proof of concept, we can work on that later if you decide you want this feature in)

@terrajobst
Copy link
Contributor

2. I don't know if it will confuse C# developers to add file when they don't need to

I think it will be useful to the subset of C# developers that disable globbing. We should probably make dotnet add file fail if globbing is enabled.

@DamianEdwards
Copy link
Member

Expanding on what @terrajobst said, I'm imagining this might be even more useful/impfactful if it was a cmd for manipulating items in the project file, with full MSBuild semantics, so you could for example do:

  • dotnet items include foo.js --type content
  • dotnet items include /content/**/*.json --type none
  • dotnet items update /**/*.xml --copy-to-output always
  • dotnet items update /**/*.json --add-attribute 'CustomAttribute="Value"'
  • dotnet items exclude /**/*.user.*

In an ideal world, this would be an MSBuild operation so it could evaluate the current state and apply the intent of the commands in the most efficient way possible, or provide warnings and errors immediately if it would result in a build error, e.g. when trying to include items that are already included and suggest doing an update instead.

@isaacabraham
Copy link

I would find this useful even in it's current state. I remove or rename a file about 1% of the time I create new files. Moving up and down could be useful - perhaps at the time of adding you select an existing file you want it to live above or below (and this could also be a move file command) but for now just dotnet add would be a nice start.

Regarding confusing C# devs, notwithstanding Immos point, perhaps we could make this only work with fsproj files and return a suitable error message.

MSBuild sounds like a bigger piece of work though 😉

@cartermp
Copy link
Contributor

For F#, adding a file should add it to the top of the scope. This can get trickier and tricker if you consider the intersection of all of these:

  • Linked files
  • Existing files
  • Ability to add above/below a given file (like in VS)
  • Adding a file to a nested scope (folder)
  • Adding a file above/below a given scope (like in VS)

It ended up being an enormous amount of code to support everything in VS.

Luckily, this isn't VS. Adding a single file to the top of the current scope that matches the directory you're in would be more than enough.

@Krzysztof-Cieslak
Copy link
Author

Krzysztof-Cieslak commented May 19, 2020

C# developers

I personally won't comment about this functionality from C# point of view, this repo is the first C# project I've opened in a few years :-)

could you mock up the help

This is the current state of help in this PR:

PS D:\Programowanie\temp\testtest> dotnet add file --help
Usage: dotnet add <PROJECT> [options] <FILE>

Arguments:
  <PROJECT>   The project file to operate on. If a file is not specified, the command will search the current directory for one.
  <FILE>      The files to add.

Options:
  -h, --help                    Show command line help.
  -f, --framework <FRAMEWORK>   Add the reference only when targeting a specific framework.
  --interactive                 Allows the command to stop and wait for user input or action (for example to complete authentication).

Optimally it would include also include something like the following options:

-a, --above <ABOVE_FILE>       Puts <FILE> above <ABOVE_FILE> in the file order. Mutually exclusive with `--below`
-b, --below <BELOW_FILE>      Puts <FILE> below<BELOW_FILE> in the file order. Mutually exclusive with `--above`
-m, --move                               Moves file entry instead of creating if the file was already added to the project file

Should the default should be before or after

I agree with @cartermp - from F# point of view, the default should be at the top of the current scope

If we have an add, do we need a remove?

Potentially yes, I guess. There also exists dotnet remove reference <PROJECT_PATH>

It actually does an include. Will it be confusing to call include add? What do we mimic, the project file or other dotnet add features?

Yes, I think add file makes sense as it mimics dotnet add reference <PROJECT_PATH>

full MSBuild semantics

Neither of other dotnet add doesn't support full MSBuild semantics as far as I'm aware - they just add the simplest case. So I'm not sure if we should complicate things here.

In an ideal world, this would be an MSBuild operation so it could evaluate the current state

As far as I understand this codebase it's not a case for dotnet add reference - so again, I'm not sure if we should do something more complex just for this single operations

@wli3
Copy link

wli3 commented May 19, 2020

I don't think dotnet items include foo.js --type content etc is too helpful. It is mostly 1:1 mapping between msbuild and commandline without any abstraction. In this case, it is the same amount of effort to edit the project file.

@wli3
Copy link

wli3 commented Jun 3, 2020

any thoughts?

@dsplaisted
Copy link
Member

Closing due to no recent activity. Feel free to open an issue or new PR to continue the design discussion.

@dsplaisted dsplaisted closed this Feb 17, 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.

8 participants