Skip to content

Comments

Fix #204 - add dfmt to the tour#516

Merged
wilzbach merged 3 commits intodlang-tour:masterfrom
wilzbach:format
Jun 10, 2017
Merged

Fix #204 - add dfmt to the tour#516
wilzbach merged 3 commits intodlang-tour:masterfrom
wilzbach:format

Conversation

@wilzbach
Copy link
Member

A very simple integration of dfmt.

@wilzbach
Copy link
Member Author

Depends on dlang-community/dfmt#281 - otherwise DUB will fail as it tries to build dfmt as an application.

@wilzbach wilzbach requested a review from PetarKirov May 22, 2017 14:05
@wilzbach wilzbach mentioned this pull request May 22, 2017
Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

First of all, looks awesome! I really like the idea and overall the implementation looks good. But unfortunately I'm getting a ton of linker errors related to dfmt. I'll investigate further and than get back to the review.

@PetarKirov
Copy link
Member

PetarKirov commented May 22, 2017

Ah, just at had a look at Travis... so the problems that I was getting locally shouldn't surprise me. My guess is that there's something wrong with the dub description of dfmt. AFAIK, most users use make to build the tools, so it's likely that the dub packages are not widely tested.

@wilzbach
Copy link
Member Author

unfortunately I'm getting a ton of linker errors related to dfmt. I'll investigate further and than get back to the review.

As mentioned that's due to dfmt having application as targetType. Once dlang-community/dfmt#281 is merged and properly tagged, everything will be green & working ;-)

@wilzbach
Copy link
Member Author

@wilzbach I've just push the 0.5.0 tag

Thanks a lot!
I have updated the PR, but it takes up to one hour for the dub registry to catch up. So in case I don't retrigger Travis, feel free to close & open this PR (or manually restart Travis).

'Ctrl-R': function(cm) {
$scope.$apply('reset()');
},
'Ctrl-F': function(cm) {
Copy link
Member

Choose a reason for hiding this comment

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

Ctrl-F is not a good choice, because on most GUI application on Windows and Linux it maps to "Find in page"

Copy link
Member

Choose a reason for hiding this comment

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

Can you map it to Ctrl+Alt+F?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, on a first attempt cfp hotkeys and the codemirror hotkeys didn't seem to support this, but "ALT-F" worked fine.
Btw there seems to be non-universal combination for formatting anyways, so I guess we are quite free in picking one, e.g:

@PetarKirov
Copy link
Member

have updated the PR, but it takes up to one hour for the dub registry to catch up. So in case I don't retrigger Travis, feel free to close & open this PR (or manually restart Travis).

Sure, I'll do some more local testing & review and then will do accordingly. When testing locally with dfmt~master, I wasn't able the formatting to work - even pressing the button did nothing. I'll investigate further tomorrow, cuz now I've got to go.

@PetarKirov PetarKirov closed this May 22, 2017
@PetarKirov PetarKirov reopened this May 22, 2017
@wilzbach
Copy link
Member Author

@ZombineDev: FYI this is green now

@wilzbach
Copy link
Member Author

@ZombineDev as I figured out how to do manual deploys, I will merge this now as it works perfectly fine on my local machine and I now have some time to watch the deploys...
Also I think seeing this in action, helps a lot for reviewing ;-)

@wilzbach wilzbach merged commit 1e68f7c into dlang-tour:master Jun 10, 2017
@wilzbach wilzbach deleted the format branch June 10, 2017 15:21
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.

2 participants