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] Roadmap update #717

Merged
merged 14 commits into from
Sep 9, 2016
Merged

[WIP] Roadmap update #717

merged 14 commits into from
Sep 9, 2016

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Sep 6, 2016

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

  • Commit message has a short title & issue references
  • Each commit does a logical chunk of work.
  • It builds and tests pass (e.g gulp tslint)

I'm going through the roadmap and see our current status. But I do find the warning tag a little bit confusing. It indicates the feature is partly implemented but we don't which part is missing: corner user case, test case or anything else? When we run into issues with commands in this category, we don't know whether it's a bug or a missing feature.

So here I did two things to the roadmap

  1. Add 🔴 to each command that requires better description of the status: which part is missing and how current command works. Part of the commands are handled by me so I'll give them a update while rest of them are implemented by @jpoon @johnfn @xconverge @sectioneight @aminroosta ,etc. Please leave comments about the status. I'll add them to the PR and then remove the 🔴 mark.
  2. Add ⬇️ to most features which rank low on our list.

After above update, we are supposed to have

  1. Most commands covered, whether it's done, partially finished with detailed description, or low-pritory.
  2. The rest of them will come soon.

:warning: | :1234: `/{pattern}[/[offset]]<CR>` | search forward for the Nth occurrence of {pattern}
:warning: | :1234: `?{pattern}[?[offset]]<CR>` | search backward for the Nth occurrence of {pattern}
:warning: :red_circle: | :1234: `/{pattern}[/[offset]]<CR>` | search forward for the Nth occurrence of {pattern}
:warning: :red_circle: | :1234: `?{pattern}[?[offset]]<CR>` | search backward for the Nth occurrence of {pattern}
| :1234: `/<CR>` | repeat last search, in the forward direction
Copy link
Member

Choose a reason for hiding this comment

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

green checkmark, this is implemented

@ascandella
Copy link
Contributor

I've looked over all the red circles and verified that I don't have updates. I believe I've kept the roadmap up to date with all my PRs. Will take another pass at my PRs tomorrow and verify there isn't something this PR is missing.

@rebornix
Copy link
Member Author

rebornix commented Sep 6, 2016

@sectioneight thanks for your quick update :)

@@ -217,8 +219,8 @@ in Visual block mode:

Status | Command | Description
---|--------|------------------------------
| I | insert the same text in front of all the selected lines
| A | append the same text after all the selected lines
:red_circle:| I | insert the same text in front of all the selected lines
Copy link
Member

@xconverge xconverge Sep 6, 2016

Choose a reason for hiding this comment

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

implemented/fixed in #720

@rebornix
Copy link
Member Author

rebornix commented Sep 6, 2016

@johnfn I noticed you converted the state of y{motion}, c{motion], etc from ✅ to ⚠️ , can you help me understand what's the catch there?

@@ -298,14 +300,14 @@ Status | Command | Description

Status | Command | Description
---|--------|------------------------------
:warning: | "{char} | use register {char} for the next delete, yank, or put
:warning: :red_circle: | "{char} | use register {char} for the next delete, yank, or put
Copy link
Member Author

Choose a reason for hiding this comment

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

@aminroosta you implement these features and I'm seeing them working well. Is there any hidden issue with it which makes you mark it as ⚠️ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rebornix I think this is compele. But there are other low priority read only registers like command line register that are not implemented.

I've left some comments in register.ts file, I will send PR updating roadmap and adding them as low priority.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your info, I'll mark it as read only registers not supported yet first :) Then you may want to give it a better description if you like.

@johnfn
Copy link
Member

johnfn commented Sep 7, 2016

@rebornix I think that might have been a mistake and I probably intended to mark yy and cc with ⚠️ . In any case they appear to be working now. (I was concerned about count prefixes)

I may have marked y with ⚠️ because I was concerned about undo/redo, but I'm more confident about that now, too.

@rebornix
Copy link
Member Author

rebornix commented Sep 7, 2016

@johnfn thanks for the input! I just added some new icons to the roadmap and the whole idea is making the status of commands more positive and instructive. A warning is somewhat confusing as people have no idea about the expected behavior of the commands.

I added below two signs

  • ✅ ⭐ telling users this command is done and it's somewhat customized with Code and we think it's better this way instead of making it totally the same as Vim.
  • 🏃 It's in progress, whether there is already a PR/issue for it or it's on our task board (mine, yours, etc).

Note 🔴 will be removed once we finish the assessment of this roadmap. I'll say we are really close in some extent (comparing to competitors on other platforms) and my next step is looking into commands which are not implemented yet and see if they need more Code's ❤️ .

@rebornix rebornix merged commit 7a21651 into VSCodeVim:master Sep 9, 2016
This pull request was closed.
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