Creation of Developer Guidance Wiki

[Colin-Stone]

We have previously discussed capturing best practices for developers of the Zowe Explorer.

I'll chip in with some points that would like the role of reviewer easier. These are suggestions only at this stage.

Review and Merge guidance to developers

• One fix per PR. No multiple fixes
• PR must relate to an issue agreed during Planning for the target milestone or in the prioritized backlog. This does not necessarily apply to external contributors as we want to encourage more contributions.
• Reviewers to prioritize based on big fixes and target milestone
• Merging will be by agreement rather than one individual? How best to manage
• No direct messaging to solicit review/merging.
• Reduce number of commits per PR. Nothing is more frustrating than to complete a review request then find a little later that several additional commits have taken place.
• If raising the PR to see if checks pass or as WIP raise as a Draft. It will not be reviewed and when you are comfortable with it you can close the draft and raise again for review

Overall code refactoring effort

The addition of an interface allows the code base to move to a more maintainable OO style based around, classes and methods rather than functions. To this end no code should be added as functions inextension.ts or the ..Actions.ts files.
Code migration is still underway to create generic command related methods and some issues experienced merging changes previously however we should now be able to manage at a function to method level now the previous changes are solidified.

Testing

• Integrate System test into pipeline build.
• Unit test code coverage. Must maintain +90% currently dropped to 91% using CodeCov may be problematic so before commiting you should check your local unit test coverage matrix. For example the % Lines for the All Files row
  
  • We should not rest until all files have >90% lines covered

[jellypuno]

System Test meaning Integration test? Or both Unit and Integration Test?

I want to chip in a few more. This is for the role of developers creating the PR.

• A more descriptive title. Issue # + Description
• Tag Issues and PRs in linked Pull Request section
• Tag the milestones
• Add labels, if possible
• Make sure that All Checks (DCO and Jenkins Run) are okay. If there are issues that you cannot handle, comment it in the PR.

For Reviewers: Let's have a minimum of 2 approvals rather than 1.

[jellypuno]

One fix per PR. No multiple fixes

@Colin-Stone Can you explain this more? 1 issue per PR?

[Colin-Stone]

One fix per PR. No multiple fixes

@Colin-Stone Can you explain this more? 1 issue per PR?

Sure. I wanted to say that we shouldn't be using PR's to include several issues in one review. It's fine if a single fix addresses several similar issues with a single fix go but they are sometimes just used to fix this and then something else not necessarily associated

[jellypuno]

One fix per PR. No multiple fixes

@Colin-Stone Can you explain this more? 1 issue per PR?

Sure. I wanted to say that we shouldn't be using PR's to include several issues in one review. It's fine if a single fix addresses several similar issues with a single fix go but they are sometimes just used to fix this and then something else not necessarily associated

okay. understood. Multiple Issues are allowed only if they are similar and connected to each other. 👍

[Colin-Stone]

@jellypuno @zFernand0 I've made a start putting this into a Wiki here. https://github.com/zowe/vscode-extension-for-zowe/wiki/Best-Practices:-Contributor-Guidance

We may want to consider adding more sections at some point or plan how we would like to use the wiki.

[phaumer]

For PRs and Branches:

• Need rules for naming branches.
• Names for maintenance branches (e.g. v.1.3.x).
• All PRs must be merged into all down stream branches before they can be closed. Developer should create branch of branch for complex down stream merges following naming conventions.

Need style sections on

• code formatting with .editorconfig, prettier or other standard formatter
• eslint rules
• modularization rules
• naming conventions for using folders, file names, class names etc.

[phaumer]

• Need rules for package-lock.json (No PRs without it included; logging of major dependency changes)
• Need rules for dependency changes and planning the roadmap (e.g. when CLI version updates will be applies; when to update versions; including typescript and node)

[Alexandru-Dumitru]

• each commiter updates CHANGELOG.md as part of their PR. It will save you (@Colin-Stone @zFernand0 ) some time before each release.
  👍 for everything else proposed so far.

[zFernand0]

For Reviewers: Let's have a minimum of 2 approvals rather than 1.

This can be enforced i the GitHub Branch protection settings 😄

• Need rules for naming branches.

How about [fix|feat]/#number? (e.g. fix/#123, feat/#234)

About the style section:

• code formatting with .editorconfig, prettier or other standard formatter
• eslint rules

Do we need to define all the rules we've decided, or is it ok to just remind people that we do have styling rules being enforced?

• modularization rules

@phaumer, How does the concept of modular styling apply here? (I'm thinking of pure imports on CSS)

• naming conventions for using folders, file names, class names etc.

Is "lower" camelCase Ok for function and property names? Same for folders names?
is "upper" CamelCase Ok for class and file names? (with the caveat that Interfaces should start with capital 'i', e.g. interface IZoweTree { ... }, IZoweTree.ts)

[zFernand0]

• each commiter updates CHANGELOG.md as part of their PR. It will save you (@Colin-Stone @zFernand0 ) some time before each release.
  👍 for everything else proposed so far.

This may be tricky because the people updating the changelog may not know the version number that will be published yet.
We have had a few discussions around this topic already and it could cause confusion if people saw new updates on the master branch changelog that are not available yet in the marketplace.

[Colin-Stone]

I do like the idea of the changelog being updated by the individuals who made the change and know it best but I agree with Fernando that the decision of what build itwill go into may not have been made

[zFernand0]

Should we have a PR process review for Wikis?

This may require a separate repository in the Zowe org. : )
@MarkAckert @jackjia-ibm, Could we get one?

[Colin-Stone]

Definitely need a review process and common agreement. I think we could add one that covers coding and style too in which we could address a number of your ealier points

[Alexandru-Dumitru]

How about looking at vscode itself for coding/styling guidelines? This should bring us some consistency with the "mothership" 👽.
Other resources from them might prove useful.
https://github.com/Microsoft/vscode/wiki/Coding-Guidelines

[Alexandru-Dumitru]

• each commiter updates CHANGELOG.md as part of their PR. It will save you (@Colin-Stone @zFernand0 ) some time before each release.
  👍 for everything else proposed so far.

This may be tricky because the people updating the changelog may not know the version number that will be published yet.
We have had a few discussions around this topic already and it could cause confusion if people saw new updates on the master branch changelog that are not available yet in the marketplace.

I understand. We could potentially have a second version that we use for active development, then have the content moved into the changelog.md during publishing (even in an automated way via jenkins). Since we are talking about establishing work processes, I guess everything goes, as long as we will agree on it.

[zFernand0]

We could potentially have a second version that we use for active development

I do agree that it may solve the immediate problem of having a person create the changelog update once a month.
However, there are other implications of changing the branching/delivery strategy. It may cause some confusion as to what the target branches of the PRs should be. It also brings up a few questions around timing and automation from active-development to master.

I'm not saying we shouldn't do it, just some things to consider.

[Alexandru-Dumitru]

Didn't knew this existed, just found it today😞, but we need to align also with Zowe general guidelines:
https://github.com/zowe/zlc/blob/master/process/CODING_STYLE.md

[Colin-Stone]

They all helps however I think we just need to disseminate and then agree as a squad the practices we want to work to. But it has to be an agreed approach.
Maybe we can pull together these different inputs and work through as a team?.

[MarkAckert]

Didn't knew this existed, just found it today😞, but we need to align also with Zowe general guidelines:
https://github.com/zowe/zlc/blob/master/process/CODING_STYLE.md

If these guidelines are interfering with the direction you want to go on guidance for this project, let me know. I'd like to understand which guidelines interfere, if they should apply here, or if they should be rewritten in the ZLC guidelines. Thanks

[phaumer]

I agree that these zlc guidelines are not a good fit and that you cannot have the same guidelines for all languages. For example, I think it is more common to have 4 spaces in TS than 2 and using uppercase constant names I think has seen its days ;-)

I would split these guidelines into two though: (1) the ones that can be automatically enforced with eslint rules and prettier and (2) the one that cannot be automated that require human reviewers such as guidelines for what is a good class and module.

(1) we should not even write down, but create an eslint file (tslint is deprecated) and review that. Stuff like formatting can be in here and enforces on Save with VS Code settings, but also rules such as requiring type declarations for all methods and function signatures, use of any, and many other rules that improve type-safety and maintainability can be enforced here. Some could lead to to errors, some just warnings.

https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin#supported-rules

[zFernand0]

Agree that we should have an eslint.config file for setting up rules.

P.S. (Personally) I like 2 spaces 😄 (but I will go with the majority on this)

[jellypuno]

Do you think it's fair, to close PRs that are open for a long time and it is not approved because they are not following the guidelines? Like it's open for 1 month and then a bot will close it with a reason?

[Colin-Stone]

Do you think it's fair, to close PRs that are open for a long time and it is not approved because they are not following the guidelines? Like it's open for 1 month and then a bot will close it with a reason?

It depends... (Sorry to give an indecisive response) The CLI squad as a whole is responsible for reviewing but in certain circumstances and speaking for myself I simply don't have the cycles to do it and feel aggrieved if I am falling behind on my own tasks. Most of us too are only part time on the squad and have other 'duties' that mean we may be able to attend the scrums only.

I think we need some "PR Champions". Who collectively represent the interests of the not only the PI items being worked on but defects and even external contributions and maybe we have a specific PR slot in the scrum when we can cover the PR list.

If we do that I think we are at least acknowledging what PR's are out there and then I think we can legitimately start culling PR's that as you describe are not following the rules or are not in a fit state to be reviewed or out of date with master etc. The responsibility is with the creator of the PR not the reviewer and I would prefer not to be assigned to a PR by the creator as it almost feels as if the responsibility of it is being transferred to me without my agreement.

[jellypuno]

Add a label "Ready for review" to let the reviewers know that all checks and requirements are okay.

[Colin-Stone]

@phaumer Do you know of any guidelines from elsewhere that could be used as a strawman to base things on. Alex mentioned these https://github.com/Microsoft/vscode/wiki/Coding-Guidelines but I think it only touches on a few of the aspects. This issue itself is touching on a lot of different areas and there are going to be many more we haven't discussed yet.