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

NodeJs Support, Project path support, Git Conflict support #33

Closed
wants to merge 14 commits into from

Conversation

sp3c1
Copy link
Contributor

@sp3c1 sp3c1 commented Apr 13, 2018

Do not know if you want to megre it or use it to make broad support. Some small features.

  • handling of NodeJs package.json
  • when in project directory, hides all the path to folder (it becomes like a home dir) - based on git path and current path comparison
  • used some of the icomoon stuff, fill free to customize (this will require you to set the icomon as the symbols font, and/or as extra font in your IDE if you want to see nice symbols)
  • stripped the -> origin from the git status check - by reordering

  • Change to modular structure that was introduced meantime on master.
  • Also added support for the Git conflict Git Conflict #35

Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
@sp3c1
Copy link
Contributor Author

sp3c1 commented Apr 13, 2018

image

@sp3c1 sp3c1 changed the title Fork changes NodeJs Support, Project path support Apr 13, 2018
@AmrEldib AmrEldib self-assigned this Apr 18, 2018
local function get_folder_name(path)
local reversePath = string.reverse(path)
local slashIndex = string.find(reversePath, "\\")
return string.sub(path, string.len(path) - slashIndex + 2)
end

local npmIcon = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this didn't get saved correctly?

Copy link
Owner

Choose a reason for hiding this comment

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

maybe encoding was messed somewhere along the way

Copy link
Contributor Author

@sp3c1 sp3c1 Apr 20, 2018

Choose a reason for hiding this comment

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

not really, set icomoon font in your ide and in your cmder
image
Dont think github is correctly handling those symbols in the code snippets

Copy link
Contributor

@mattdkerr mattdkerr left a comment

Choose a reason for hiding this comment

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

encoding issue?

@AmrEldib
Copy link
Owner

I should mention that I've been working over the past few days (since this PR came in) on a refactor that makes it much easier to add 'addons' like this.
The goal is offload all the clink work to one 'core' file, and only have the addon file (for git, hg, npm, python, etc) call one function to do all this work.
This will also allow adding multiple segments to the prompt for folders that have, say, npm and git, without either addon having to be aware of the other.

@mattdkerr
Copy link
Contributor

Sounds great- I could then write in an add-on for #30

@AmrEldib
Copy link
Owner

The Git segment is pulled into its own addon. Only few changes are needed to make #30 happen. It should be good.
I'm also considering adding configurations to let people control some things without changing code

@AmrEldib
Copy link
Owner

I've published the refactored version. I've added an explanation in the core and addon files, but README and a proper docs file are still to be edited. Maybe this weekend, along with merging this npm addon.

@sp3c1
Copy link
Contributor Author

sp3c1 commented Apr 20, 2018

cool, want me to resolve the conflicts with base branch?

@sp3c1
Copy link
Contributor Author

sp3c1 commented Apr 20, 2018

@AmrEldib going through new structure and I like it. Was thinking to write one myself like that but was too lazy ;]

Will port changes to that new format, also will extract variables to the config, update the sample file and delete from repo the actual config

Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
@sp3c1
Copy link
Contributor Author

sp3c1 commented Apr 21, 2018

Think thats it, @AmrEldib @mattdkerr review again please

-- Sets the properties of the Segment object, and prepares for a segment to be added
---
local function init()
segment.isNeeded = get_package_json_dir()
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be preferable for get_package_json_dir() to be named something starting with "has"/"is" that indicates it returns a boolean result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as it actually returns something from io.openit should not be prefixed has, I think

@@ -5,6 +5,9 @@
local promptTypeFull = "full"
-- "folder" for folder name only like System32
local promptTypeFolder = "folder"
-- "smart" for treating git top level folder as homeSymbol
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

end

segment.textColor = colorWhite
segment.fillColor = colorCyan
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we customize this more easily? I can execute clink and this lua under Hyper or Cmder and they each have their own color theme systems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ve actually tried moving that to config, but it has to be processed after the core and throws errors. Not proficient enough in lua to work around it ;p (and to tell the truth I didnt really look hard)

That said, you can change your color directly in cmder settings can you. And I would actually say this should be probably preferable way to change color themes anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

what kind of customizations are you thinking of?
The order of files does affect results. The only ways to force order I can find (but not 100% sure) are:

  • file names: this is why the config file starts with an underscore, to be loaded before all other files
  • The number passed to the register_filter function. The higher the number, the later the function is called. This way the folder name can always be first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about actually putting colors into the config. As cmder can handle those extra color codes, but then we could put it in core as well. So i dont know if it is even worth it to extract that.

clink.prompt.register_filter(addAddonSegment, delayGit)
Copy link
Owner

Choose a reason for hiding this comment

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

@sp3c1 what is delayGit? and delayNpm?

Copy link
Contributor Author

@sp3c1 sp3c1 Apr 24, 2018

Choose a reason for hiding this comment

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

variable in config - forgot to update the sample one, will send the update
👍

end
end
else
local git_dir = get_git_dir()
Copy link
Owner

Choose a reason for hiding this comment

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

There's an unobvious issue here. get_git_dir is a function is a lua file shipped with Cmder. This function gets overwritten in the powerline_git file. However, I think here we're calling the original function shipped with Cmder which could cause issues later if it changes, or if the powerline_git function diverges from Cmder's
This highlights that powerline_prompt depends on powerline_git, but powerline_git executes after powerline_prompt

Copy link
Contributor Author

@sp3c1 sp3c1 Apr 24, 2018

Choose a reason for hiding this comment

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

that is a piccle, as the git component is quite crucial.

maybe we move it to very beginning to override it so that no addon gonna have same issue. No its not as clean at it could be, but would resolve it and ensure it wont randomly break?

@AmrEldib
Copy link
Owner

@sp3c1 @mattdkerr I appreciate all your work on this.
I do have some questions:

  • The symbols (which are very nice) require the icomoon font. Either we find symbols that don't require an additional font, or we put clear instructions how to obtain and install this font to make these symbols show up correctly.
  • I suggest replacing the VS Code symbol with a more generic code symbol like "<>" for people who are not using VS Code as their IDE
  • I see that constants were moved to the config file. I like giving users control over which symbols to use, but I'm not sure the arrow symbol should be configurable, because the code relies on how it looks.

@sp3c1
Copy link
Contributor Author

sp3c1 commented Apr 24, 2018

@AmrEldib

  • Regarding the icomoon I would say lets add isntruction and even screenshot how to set it.
  • Regarding VS Code, its already changed to git icon - as this effectively works with any git project
  • Yeah, will move back the arrow (though it would give use ability to use some other arrows etc, but we can keep it locked)

@sp3c1
Copy link
Contributor Author

sp3c1 commented Apr 24, 2018

Regarding fonts, maybe we should link the https://github.com/ryanoasis/nerd-fonts? I mean anyone should pick whatever he really likes

@sp3c1
Copy link
Contributor Author

sp3c1 commented Apr 25, 2018

Or actually set this to some normal font in repo with screenshot instructions how to set it to something different?

Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
@sp3c1 sp3c1 changed the title NodeJs Support, Project path support NodeJs Support, Project path support, Git Conflict support Apr 27, 2018
Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
@sp3c1
Copy link
Contributor Author

sp3c1 commented May 1, 2018

@AmrEldib think that covers it

@AmrEldib
Copy link
Owner

AmrEldib commented May 1, 2018

@sp3c1 I'm working on this, but been a little busy lately. Thanks again for all your help

@sp3c1
Copy link
Contributor Author

sp3c1 commented May 1, 2018

np ^^

Signed-off-by: Bartlomiej Specjalny <bspecjalny@gmail.com>
@AmrEldib
Copy link
Owner

Sorry for the unplanned delay. I merged all the addon after making some changes to make sure the code adhere to the same style and is all documented.
I've updated the README and screenshots.
I made some changes to how the smart prompt works. It'll show the folder name instead of just the project. This allows showing the folder name when navigating to subfolders.
Thank you @sp3c1 and @mattdkerr for all your help. Much appreciated!

@AmrEldib AmrEldib closed this May 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants