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

InstantiationService debt #5443

Merged
merged 14 commits into from
Apr 18, 2016
Merged

InstantiationService debt #5443

merged 14 commits into from
Apr 18, 2016

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Apr 18, 2016

No description provided.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @egamma, @aeschli and @alexandrudima to be potential reviewers

@jrieken jrieken self-assigned this Apr 18, 2016
@jrieken jrieken added this to the April 2016 milestone Apr 18, 2016
@jrieken jrieken merged commit 620e8b7 into master Apr 18, 2016
@jrieken jrieken deleted the joh/inst branch April 18, 2016 12:49
@outcoldman
Copy link
Contributor

@jrieken two questions about the workflow you are using for development (not really related to this PR, just want to start discussion and open a thread later if we will need):

  • looking on this PR - it seems like that you (and I guess some more folks) are allowed to merge changes without getting code review approvals. Is that right?
  • right now the history is really messed up. For example currently it just hard to understand what kind of work has been done on April 18. A lot of commits from you and @isidorn. Why not require people to squash commits before merging?
  • descriptions for PR and commits are not helping with code reviews. Most of the commit messages are "fixes blah", "removing foo", "speed up bar". I believe that good commit messages will help us, contributors and users, to understand what to expect from the next release, what is the priority bugs/issues you are working right now and producing really good release notes.
  • code commentaries. sadly there are 90% of the code without any commentaries. It just really hard for new people to understand how all of the parts are working.

@jrieken
Copy link
Member Author

jrieken commented Apr 18, 2016

Thanks for the feedback. I hope I can provide some answers:

looking on this PR - it seems like that you (and I guess some more folks) are allowed to merge changes without getting code review approvals. Is that right?

That's correct. We all work against master and don't do pull requests so often. This works well for us because we self host very aggressively - it happens we update alpha a few times a day. So people will notice breakage quickly and there is no hesitation to revert a change.

Usually changes become less wild towards the end of a month. That is because we start a new iteration every month which usually starts with one week debt/exploration work, two weeks feature work, and one week testing/stabilisation. Then we might do pull requests, reviews, and/or program in pairs.

right now the history is really messed up. For example currently it just hard to understand what kind of work has been done on April 18. A lot of commits from you and @isidorn. Why not require people to squash commits before merging?

I like that suggestion. Esp. with the recent introduction of merge&squash we should do this more often. It's not a rule but everyone should include an issue number when pushing. When doing debt work that often gets forgotten and we should improve on that.

descriptions for PR and commits are not helping with code reviews. Most of the commit messages are "fixes blah", "removing foo", "speed up bar". I believe that good commit messages will help us, contributors and users, to understand what to expect from the next release, what is the priority bugs/issues you are working right now and producing really good release notes.

The work is tracked in our monthly plans. My recent debt work on service injection got forgotten because it spun-off a water cooler discussion and I just jumped right on... More on our workflow can be found here: https://github.com/Microsoft/vscode/blob/master/wiki/project-management/development-process.md

code commentaries. sadly there are 90% of the code without any commentaries. It just really hard for new people to understand how all of the parts are working.

We provide an birds eye view on how stuff is organised and I'd love to receive some feedback. Because doc comments are usually outdated quickly we have decided to use only TypeScript's type annotations for our internal APIs.

fyi @egamma

@outcoldman
Copy link
Contributor

@jrieken thank you for answers! It helped to understand the process.

I am new for VS Code, liked the concept of really simple editor with few advanced options. Gives me some thoughts of switching from Vim to VS Code. The main pros for this are:

  • Being open source.
  • Nice code (really good job here) - easy to debug and maintain. (comparing to Vim codebase...)
  • Having a way to easily fix some annoying bugs without bugging you. Just hope that process of approval is clear and simple.
  • Being open for the future iteration (April Iteration Plan #4888).

@jrieken you said "it happens we update alpha a few times a day" - how to get on alpha channel?

@egamma
Copy link
Member

egamma commented Apr 19, 2016

how to get on alpha channel?

Currently the alpha channel is internal. Now that 1.0 is out we will revisit this.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants