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

switch build system and structure according to community best practises #30

Closed
wants to merge 5 commits into from

Conversation

fbehrens
Copy link
Contributor

@fbehrens fbehrens commented Jan 4, 2018

No description provided.

@forki forki requested a review from tomasaschan January 5, 2018 07:14
@forki
Copy link
Member

forki commented Jan 5, 2018

@tlycken - what do you think? Since you are mentioned as build/infrastructure in readme I think you decide on such stuff :P

Copy link

@tomasaschan tomasaschan left a comment

Choose a reason for hiding this comment

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

@fbehrens I'm really sorry if I'm being harsh here - I really appreciate the efforts you've put in to make this perfect. But, I think this is the wrong approach. Admittedly, this scaffold probably has some good things that my scaffold doesn't - but the converse statement is also true.

If there are specific things in the current build infrastructure you want to change, because they, for some reason or other, are pain points, then go ahead and file pull requests for improvements in those areas. For example, if it's a hassle to do ./build.sh -- clean from within the src directory instead of ./build.sh clean, feel free to submit a patch to the build.sh script that decides whether to use mono in a different way. Another example: if you're unhappy with a project name, submit a patch with only that change.

As it stands, this PR does too many things at the same time for any of them to be evaluated on their own merits. This is what I meant with the last paragraph here; a complete remake (which this PR is) is, IMO, a waste of time and should not be merged.


Getting into specifics, there's also a couple of things I dislike with this structure, compared to the one I created (which is why I did it the way I did it, and not exactly like the SAFE example):

  • It clutters the repository quite a lot to have all the build tooling dumped in the root
  • Test code ends up quite far away from the code it tests, meaning you have to navigate further (mentally and in your file system) to edit tests and code together. My gut feeling is that this project will never be huge, and thus separating into src and tests is overkill; there simply won't be enough code to motivate that granularity. (And if I'm proven wrong in the future, it's not difficult to change then.)
  • The project names no longer convey anything about what type of projects they are (which one should I look at to find the actual application logic? It's not obvious from the names), nor do they conform to the way things are "usually" structured in the .NET world (named Project.Subsytem.Whatever etc) but have rather arbitrary names.

@fbehrens
Copy link
Contributor Author

fbehrens commented Jan 5, 2018

@tlycken Nevermind. I did not put a lot effort into this. All I did is create three empty projects Mechanic for the library, Mech for the cli-wrapper using Argu and MechanicTests using Expecto, including them in a sln and setting the project dependencies using the dotnet command. I proper initialized paket and pulled in and copied the build scripts from suave-bookstore (with some reducing in fsx).

The point of this PR are not the project names (They just happen to were the names i would have come up with) nor the separation of test and src code (folderstructure is flexible).

The main point is to have paket.dependencies,.paket,packages and build.* in root like all other fsprojects and use the battle-tested shell-wrappers from @forki.

I am still willing to just do this main in single litte clean green PR

PS i don't understand your argument about cluttering the root directory. This is a widely acceted standard. It doesn't make it better to put all those files in a subdirecory.

@jindraivanek
Copy link
Contributor

There is practical problem with paket.dependencies outside root: Ionide Paket integration don't work.

@tomasaschan
Copy link

@jindraivanek Ah, that I didn't know. And that's definitely a good argument to change. Does the plugin work if you use the src folder as workspace root?

(Side note: I'm tempted to file an issue to the ionide paket package...)

@donopj2
Copy link
Contributor

donopj2 commented Jan 7, 2018

I've been working out of the src folder in VSCode and Ionide-Paket has been working fine for me

@jindraivanek
Copy link
Contributor

@tlycken

Does the plugin work if you use the src folder as workspace root?

yes.

Ionide behave the same as Paket runned in workspace root. If you try to run Paket in project root you get:

-> Could not find 'paket.dependencies'. To use Paket with this solution, please run 'paket init' first.
   If you have already run 'paket.init' then ensure that 'paket.dependencies' is located in the top level directory of your repository.
   Like this:
   MySourceDir
     .paket
     paket.dependencies

I think paket.dependencies and .paket in root dir is standard in F# repo with Paket, and other location can be confusing and hard to use.

@forki
Copy link
Member

forki commented Jan 8, 2018 via email

@tomasaschan
Copy link

Given that there seems to be good tooling reasons to do so, I've changed my mind and is also in favor of moving build tooling to the root.

I'm still against doing it with a PR like this, though; a change like that tends to touch files all across the repo, so to make it easy to review it should ideally do nothing except move files around, and, where it's necessary, change path references. The files themselves should (except for the mentioned path ref changes) not change at all.

@tomasaschan tomasaschan closed this Jan 8, 2018
@fbehrens fbehrens deleted the build_system branch January 8, 2018 21:36
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.

5 participants