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

[WIP] Move Clojure Warrior into Calva #362

Merged
merged 56 commits into from
Oct 1, 2019
Merged

[WIP] Move Clojure Warrior into Calva #362

merged 56 commits into from
Oct 1, 2019

Conversation

PEZ
Copy link
Collaborator

@PEZ PEZ commented Sep 30, 2019

This is just started.

When it's done it will have Introduced the following change(s):

  • Moved Clojure Warrior source code into Calva
  • Made Calva contribute what CW previously contributed (settings, commands, etcetera)
  • Made Calva activate the CW features such as rainbows and comment dimming/highlighting

I have:

  • Read How to Contribute.
  • Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Updated the README.md
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

Ping: @cfehse , @kstehn

@PEZ
Copy link
Collaborator Author

PEZ commented Sep 30, 2019

This is ready for merge afaict, @kstehn and @cfehse . What do you say?

@cfehse
Copy link
Contributor

cfehse commented Sep 30, 2019

@PEZ I take a look at this tomorrow, okay?

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

@PEZ If I checkout the branch, build and run the build in debug - the (comment...) form are no longer dimmed. Original Clojure Warrior Extension is not installed. I changed nothing in my settings.

image

See below: it's a feature.

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

@PEZ Wouldn't it be better to really integrate the code into the calva namespace. Renaming everything from "Clojure Warrior" into "Calva Warrior" or better to "Calva <whatever it's called, what the extension actually does>"? Now it is a bit of a mishmash to me.

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

@PEZ @kstehn

Perhaps we should rearrange the source directory as well to something like that:
src
src/calva
src/cljs-lib
src/calva-fmt
src/calva-warrior
and then get rid of the src/'ts directories inside the modules as well.

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

@PEZ @kstehn
calva-fmt and calva-warrior should really be one module, shouldn't they?

@kstehn
Copy link
Contributor

kstehn commented Oct 1, 2019

Yeah i like the idea of the rearrange
there are some things that need some restructering in calva itself

@PEZ
Copy link
Collaborator Author

PEZ commented Oct 1, 2019

the (comment...) form are no longer dimmed

That's because I changed that so that they are now italicized instead. 😄 Ignore forms are still dimmed.

@PEZ
Copy link
Collaborator Author

PEZ commented Oct 1, 2019

Regarding naming, I'm pretty happy with calling it Clojure Warrior. I don't want to change the settings names, so then it makes sense to call the module something similar to what the settings say.

Regarding rearranging, I am all for that, but it is a different issue than this PR, I'd say.

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

Regarding naming, I'm pretty happy with calling it Clojure Warrior. I don't want to change the settings names, so then it makes sense to call the module something similar to what the settings say.

But how does a user now, that these "strange" Clojure Warrior settings belong to the Calva extension? In my experience with users things should be named clearly and comprehensibly. Why not name this Calva Code Formater (Clava-fmt) and Calva Rainbow Brackets (Clojure Warrior). I personally never heard of Clojure Warrior before I installed Calva the first time. And what about old settings with the same name left over from an installation of the "old original" extension - namespace clash? - or does vscode remove all this entirely?

Regarding rearranging, I am all for that, but it is a different issue than this PR, I'd say.

Yes but it should be done not too long after the merge I think.

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

That's because I changed that so that they are now italicized instead. 😄 Ignore forms are still dimmed.

That's good! I - and my eyes - like that much better. gg

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

@PEZ

There are some unused imports and linter warnings (strangely the import * as isEqual from 'lodash.isequal'; is even shown as an error).

image

@PEZ
Copy link
Collaborator Author

PEZ commented Oct 1, 2019

what about old settings with the same name left over from an installation of the "old original" extension - namespace clash?

That's what I meant. I want people's old settings to continue to work. Which they do now. We can discuss how wise that is with regards to better names, but at least we are not making anything worse this way. 😄

@PEZ
Copy link
Collaborator Author

PEZ commented Oct 1, 2019

import * as isEqual from 'lodash.isequal'; is even shown as an error

npm i?

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

npm i?

No - I tried that first. The npm page for lodash.isequal recommended the require('lodash.isequal') notation as well. I have no idea why. gg

@cfehse
Copy link
Contributor

cfehse commented Oct 1, 2019

@PEZ @kstehn This is ready to merge, isn't it?

@kstehn
Copy link
Contributor

kstehn commented Oct 1, 2019

At a glance seems to be fine to me :)

@PEZ
Copy link
Collaborator Author

PEZ commented Oct 1, 2019

I decided that it was better to change the name now, rather than later. It is now called ”Calva Highlight”, since that is closest to what it provides. We should probably move the grammar things to this module/subdirectory as well, but that is for later.

In order to not just abandon the users with silently non-functioning settings, I have added a detector for oldly named settings and flash a warning message when those are detected. The message includes information on what the user should now do. To test this, add some settings that are valid for calva.highlight, but prefix with clojureWarrior instead.

@PEZ PEZ merged commit 432d98a into dev Oct 1, 2019
@PEZ PEZ deleted the wip/clojure-warrior-inline branch October 1, 2019 18:05
@PEZ
Copy link
Collaborator Author

PEZ commented Oct 1, 2019

Merged it since you guys were OK with it and I just change things according to previous feedback. Please be alert on any problems that could be related to this change when you have pulled dev.

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