Closed tehraninasab closed 1 year ago
I would rename the title to "Convert launcher.fsx to a project"
Please note, I didn't say "PR title".
BTW, I saw you pushed 1 by 1, and I don't understand why. If you're addressing my feedback in different commits, that's fine, but as soon as you see CI green on them, there's no point to not squash everything into 1. Therefore, pushing 1 by 1 when you're going to squash later, doesn't make much sense (it just clogs your Actions tab of so many unnecessary CI jobs).
I would rename the title to "Convert launcher.fsx to a project"
Please note, I didn't say "PR title".
BTW, I saw you pushed 1 by 1, and I don't understand why. If you're addressing my feedback in different commits, that's fine, but as soon as you see CI green on them, there's no point to not squash everything into 1. Therefore, pushing 1 by 1 when you're going to squash later, doesn't make much sense (it just clogs your Actions tab of so many unnecessary CI jobs).
The reason is each CI run takes about 20 minutes and I didn't want to wait 20 mins to apply the next comment :thinking:
I didn't want to wait 20 mins to apply the next comment 🤔
By clogging your github account with so many unnecessary jobs, you caused an even bigger wait than 20min in my opinion.
BTW the issue is not so much about pushing 1 by 1, the thing I would have not done, myself, would be to do git rebase -i
if you know already that these commits will be squashed with the 1st one (as they don't have any reason to leave separated from the 1st commit).
I didn't want to wait 20 mins to apply the next comment thinking
By clogging your github account with so many unnecessary jobs, you caused an even bigger wait than 20min in my opinion. BTW the issue is not so much about pushing 1 by 1, the thing I would have not done, myself, would be to do
git rebase -i
if you know already that these commits will be squashed with the 1st one (as they don't have any reason to leave separated from the 1st commit).
Right
Let's also remove this line from the ReadMe file as part of the commit of this PR: https://github.com/nblockchain/fsx/blame/master/ReadMe.md#L10
After you have addressed all the feedback, this PR needs to get rebased. (But please squash to 1 commit before rebasing; this way the git rebase
command will potentially pose less conflicts to deal with, if any.)
Let's also remove this line from the ReadMe file as part of the commit of this PR: https://github.com/nblockchain/fsx/blame/master/ReadMe.md#L10
Done
After you have addressed all the feedback, this PR needs to get rebased. (But please squash to 1 commit before rebasing; this way the
git rebase
command will potentially pose less conflicts to deal with, if any.)
Done
Looking great! Now, let's finish the original task by renaming the PR to "Publish fsx in nuget", and have a 2nd commit (do not squash with 1st), that publishes the fsx.fsproj to nuget.org in the same way you did with fsxc.fsproj.
Ok good, the 2nd commit might have looked easy enough to do, but I'm afraid that it will not be as easy as you think. Why do I say this? Because I'm sure that, if published as is, it would fail. The reason is that fsx has a dependency on fsxc, but currently that dependency is a runtime-dependency. That is, fsx tries to look for fsxc.dll (or fsxc.exe) in the same folder where fsx.dll (or fsx.exe) is located. This won't work when being installed as dotnet nuget tool. Even if you add a <PackageReference>
to fsx.fsproj, I doubt it would work either, so I'm afraid that you would need to do the following instead:
Last but not least, when you have the above finished and working (CI green), please remove this line from the ReadMe in the 2nd commit: https://github.com/nblockchain/fsx/blame/master/ReadMe.md#L9
This is almost ready! good work, don't despair, will review couple of things more.
Last but not least, I would rename the title to "Convert launcher.fsx to a project". After addressing all my feedback, we would be much closer to finishing this task, good work so far.