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

Items #299, #300, #301 #302 and #304 #303

Closed
wants to merge 10 commits into from
Closed

Items #299, #300, #301 #302 and #304 #303

wants to merge 10 commits into from

Conversation

jhoneill
Copy link
Contributor

@jhoneill jhoneill commented May 2, 2020

PR Summary

PR Checklist

  • [ X ] [Write Help]
    Small updates to the existing help
  • [ O ] [Write Unit Test]
    Have not added tests for new functionality
  • [ X ] [Update CHANGELOG.md]
    As above. Though above had an extra read through

@jhoneill jhoneill changed the title Items #299, 300, 301 and 302 Items #299, #300, #301 #302 and #304 May 3, 2020
@SebastianSchuetze
Copy link
Collaborator

@jhoneill thanks for the PR. In the future could you please not have so many changes in one PR. It does not speed up a review. It is quite the opposite. One PR for each issue and usually each issue should try to handle one use case or topic.

Would it be possible for you to split them up? I know it is some more work for you, but it reduces work/review packages and makes it easier to understand.

@jhoneill
Copy link
Contributor Author

jhoneill commented May 4, 2020

I can try to do that in future. I try to keep to one commit per area.
I'm about to send you ~20 new commands (Add-Process, Add-WorkItemItemType, and stuff to create fields and layout workitem forms) and it is fairly difficult to break those into small pieces .
Because the parts depend on each other if I send you (for example) may update for processes as one PR, and the update for work item types as second, PR #2 (a) contain all of PR #1 and (b) Needs to come from a new branch (if I commit into the branch I sent in PR1 that github adds them to the open request i.e. the pull by branch not by commit).

If you like I can roll back to
Reduce scope of default -Project parameter to *-Vsteam* commands make a branch and a PR
then to
Change formats.json to include all .ps1xml files in formats and do the same
and then to
Allow Get-VSteamWorkitemType to use cached process info and to expand
then
and do the same for the last 3. Does 6 PRs with each one depending on those before it help you ? My hunch is not. Because if you change something I did in the first when it is merged, it then needs to be changed in the other five. So I'd assumed that one PR with work in multiple commits would show what depended on what else. If that's not working for you , let me know and I'll try to adapt.
Cheers
J

@jhoneill
Copy link
Contributor Author

jhoneill commented May 6, 2020

Do let me know what your preference is.
Including help, which means commands get 3 files, and formatting information what's in this PR plus the next block of work is ~ 120-130 new or changed files. They divide into ~19 sections (got a spreadsheet with that in front of me). I can withdraw this one this one and make 19 branches and 19 PRs , or keep this one and send you the additions as one or two PRs with a list of what's different and why. The latter is less work for me, but I don't mind doing that work if it helps you.

@SebastianSchuetze
Copy link
Collaborator

Thanks for your tips and effort. I have not much time currently as I am doing this in my free time. This is not part of my daily job.
I will think about it as soon as I have some time to think about it.

@jhoneill
Copy link
Contributor Author

jhoneill commented May 6, 2020

No worries. This came about as a by product of my day job :-) But getting it fit to share is/was my own time.

@DarqueWarrior
Copy link
Collaborator

DarqueWarrior commented May 13, 2020

You can't do this.

Change formats.json and types.json to list "*" instead of individual files. This won't work for classes because they must be loaded in the correct sequence

The order of formats is important. The tables have to be listed first. If you do a * it will be in alphabet order and the list formats will be first which is not what I want. The Readme.md in the folder explains why this can't be done.

I also think your branch must be way behind because types in master has already been changed to a * format.

Please make sure you have the latest changes in your branch.

In the future please make smaller PRs so we can see each change and not have to reject the entire PR.

@DarqueWarrior
Copy link
Collaborator

This change has already been made

Change scope of default -Project parameter to apply only to -Vsteam commands (#297 - impacts _clearEnvironmentVariables, get-VSTeamInfo *-VSTeamDefaultProject)

@DarqueWarrior
Copy link
Collaborator

Already fixed

Change _callAPI to trap being called when no logon/account is set.

@DarqueWarrior
Copy link
Collaborator

Change Invoke-vsteamRequest. (#302) Switch order of parameters "area" and "resource" and give them completers. Add a switch '-ExpandValue'
Example: Type Invoke-vs [Tab] [-] [Tab] "-area" completes (first on the url path) [space] [Tab] "build" etc complete; for "WIT" -Resource [Tab] will suggest "workitemtypecategories" etc. add -value or -ExpandValue and instead of Count=15; Value=[Array], the array is returned

You don't have to change the order. This will be in next version.

@jhoneill
Copy link
Contributor Author

Change formats.json and types.json to list "*" instead of individual files.

The order of formats is important. ....
The Readme.md in the folder explains why this can't be done.

My apologies. I missed that and was trying to take a short cut. I will revert to the approved way.

I also think your branch must be way behind ...

Please make sure you have the latest changes in your branch.

I had sync'd up to date I put the the PR in. It looks more things have gone in since.
Last that I saw, you were doing the change of scope for the default parameter but it hadn't gone in, and I did the catch for no active logon/account raised it as an issue, and did the PR in a very short space of time, and we seem to have duplicated each other there.

Change Invoke-vsteamRequest. (#302) Switch order of parameters "area" and "resource" and give them completers. Add a switch '-ExpandValue'

You don't have to change the order. This will be in next version.
OK I'll back this out.

In the future please make smaller PRs so we can see each change and not have to reject the entire PR.

Yes. I'm sorry for sending such large lump. For replacing dynamic parameters with the completers and validator classes that was unavoidable. It is far to easy for me to lapse into "While I am doing X I will just fix Y", which feels like being helpful, but isn't always a good way to deliver help.

I have a big block of work which I am waiting to send you. We got asked to configure some ProcessTemplates in other words, "Add "Scrum 2" as our version of scrum, add work item types to it, set up layouts, fields, statuses, boards for the those work item types. Make it almost the same as this other DevOps Org, but change this to that ... etc.".
We ended up doing "Process Template as code" :-) and I wrote these new functions

Add-VSTeamField
Add-VSTeamPickList
Add-VSTeamProcess
Add-VSTeamProcessBehavior
Add-VsteamWorkItemControl
Add-VsteamWorkItemField
Add-VsteamWorkItemPage
Add-VsteamWorkItemPageGroup
Add-VsteamWorkItemState
Add-VSTeamWorkItemType
Get-VSTeamBuildTimeline
Get-VSTeamField
Get-VSTeamPickList
Get-VSTeamProcessBehavior
Get-VSTeamWorkItemBehavior
Get-VsteamWorkItemField
Get-VsteamWorkItemPage
Get-VsteamWorkItemState
Hide-VsteamWorkItemState
Remove-VsteamWorkItemState
Remove-VSTeamWorkItemType
Set-VSTeamPickList
Set-VSTeamProcess
Set-VSTeamWorkItemBehavior
Set-VSTeamWorkItemType
Show-VsteamWorkItemState

They leverage the completers / validators I have already given you and add some more, and in doing the new work, I found the existing completers should wrap some values in quotes and the validators should not reject null or empty (if a parameter is mandatory Null/empty needs to be explicitly allowed and if not it needs to be forbidden and that's not the validator's job).

I have also moved the responsibility for managing the object caches into their classes so they now have Update, GetCurrent (which will update if the cache is empty or stale) and Invalidate to force the next cycle to update. Where you had a method for determining "stale" I have left it, and the properties remain exposed for anything which relied on directly accessing them.

Things like Get-Process now update the cache if they do a "fetch all" using that and I made another change to get process : there are two different places you can call for process information and I wanted the richer one as a pre-req for the new commands.

What this PR was trying to be was "the changes to the already released things which are pre-requisites for the new functions above". But I seem to have made more problems than benefits. So I propose to

  1. Close this this PR.
  2. Sync what I have with your master branch. Currently I have "official" which is your master, "Master" which is this PR and a feature branch with all the extra changes in it. Some re-arrangement of my branches is going to be needed.
  3. Back out my changes to the parameter order of Invoke, and to how things are built
  4. Send you a new PR with only the updates to the completers/ associated cache classes, and the new completers for Invoke-, you are changing Invoke so you can add the completers or not in doing that.
  5. File a new issue saying "Build the classes into the PSM1" with the changes required. I will leave you to change build.ps1
  6. Send you my changes to Get-VSTeamProcess and to the Process Object as a separate PR - it depends on the completer updates, and the long list above depends on it.
  7. Send you my changes to Get-VSTWorkItemType again the same dependencies.
  8. Send you the new commands. **What would be the best way to send the changes ? **. There are two dozen functions each with a PS1 and 2 .MD files, plus some format files, plus some additional classes so the total is getting up near 100 files, and I think that gives Sebastian nightmares :-) I can group as
  • Processes
  • WorkItem types & their states
  • "Behaviors" (boards) for processes and work item types
  • Fields and picklists
  • Associate fields with WorkItem-types and laying them out

I think that ends up as 8 PRs. Does that work for you ?
Best etc.
J

@SebastianSchuetze
Copy link
Collaborator

SebastianSchuetze commented May 13, 2020

I think redoing the PR would help us much better.

8. Send you the new commands. **What would be the best way to send the changes ? **. There are two dozen functions each with a PS1 and 2 .MD files, plus some format files, plus some additional classes so the total is getting up near 100 files, and I think that gives Sebastian nightmares :-) I can group as

The best way to do it I think if keeping related files together. Look at the PR #286 which was grouping cmdlets for classification nodes. You could do the same for the cmdlets.

E.g. include in the same PR

  • Add-VsteamWorkItemState
  • Get-VsteamWorkItemState
  • Hide-VsteamWorkItemState
  • Remove-VsteamWorkItemState
  • Show-VsteamWorkItemState
  • All documentation files for the cmdlets
  • format files where the format.json should only have these files included. Other following cmdlets should then be added later
  • type files for the cmdlets
  • classes you created (classes are not definitely always needed, since it is a feature for PSDrive option)

make sure they follow the same style as in my PR earlier.

If you have changes that apply to multiple cmdlets then they should come early included but they should not be dependant from anything. Meaning they should work without the changes coming later that are dependant.

Also make sure first that our pipeline is going through with all environments. We do not usually start to review before the pipeline is not succeeding.

And thanks for your understanding. Because most of the problem in open source is:

  • change only related things and not multiple different things on the way
  • different views
  • different styles
  • due to limited time long PRs wait long time and can get outdated or will rather be rejected than included. Even if they are awesome
  • many people depend on your changes. If you change things to easily many people can be affected if shit hits the fan. And some people even make money with this module.

@DarqueWarrior
Copy link
Collaborator

I already wrote the completer for Invoke last night and is already in master.

@DarqueWarrior
Copy link
Collaborator

I like the idea of grouping PRs around a noun "WorkItemState" all the verbs, tests and doc for that noun can be a single PR.

@DarqueWarrior
Copy link
Collaborator

Finally there is no need to apologize for helping. We really appreciate your support and understanding.

@SebastianSchuetze
Copy link
Collaborator

Fully agree with @DarqueWarrior. Help is much appreciated since most of it is a personal effort and the drive to improve things.

@jhoneill
Copy link
Contributor Author

Thanks both, I'll stop saying sorry. It's not that I'm talking down what I've done, just aware that I could have delivered it a way that's more convenient for you. And sometimes explaining what one has done with only text to go on, it can look like "I AM RIGHT BECAUSE" so I've have taken to saying "Whoops, I ..." "My bad, it was trying to..." "Sorry, I thought ..." to save accidentally causing an argument. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment