Skip to content

Conversation

@vors
Copy link
Collaborator

@vors vors commented Mar 23, 2016

There are 2 functions:

  • New-MappingFile - creates mapping.json from project.json files
    (one is included in this commit)
  • Copy-SubmoduleFiles - enables copying files back and forth from
    submodule to src/ based on mapping.json file

This change is Reviewable

@andyleejordan
Copy link
Member

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions.


mapping.json, line 0 [r1] (raw file):
I hate littering the root directory with more stuff. Do we want to add a folder for things like this? We have a lot of scripts in the top level too; but OTOH, moving them means fixing the scripts to run from the right directory. I might just need to ignore this.


PowerShellGitHubDev.psm1, line 349 [r1] (raw file):
I've been here... You might be a lot happier just using Json.NET, like so.


PowerShellGitHubDev.psm1, line 395 [r1] (raw file):
Backslash won't work on Linux.


PowerShellGitHubDev.psm1, line 398 [r1] (raw file):
Can we have a loud confirmation/warning that these are destructive operations, and will overwrite all existing files and their modifications?

Perhaps a check to git status to see if the tree is dirty, and if so, abort.


PowerShellGitHubDev.psm1, line 412 [r1] (raw file):
We should probably handle file encodings here. Let's get everything into the superproject in a consistent, correct encoding (which makes copying them back a pain since the source files' encodings are inconsistent; we may need to track this in mapping.json).


PowerShellGitHubDev.psm1, line 436 [r1] (raw file):
Backslash won't work on Linux.


PowerShellGitHubDev.psm1, line 441 [r1] (raw file):
Is this manually skipping these? There is a new project in #734, TypeCatalogParser.

I wanted to suggest using Microsoft.DotNet.ProjectModel to get the compileFiles more easily; but really it just makes all of SourceFiles available; and you're needing to exclude things in compile but not compileFiles.


PowerShellGitHubDev.psm1, line 450 [r1] (raw file):
Do you need to skip the Microsoft.PowerShell.Linux.Host and SecureString projects too?


PowerShellGitHubDev.psm1, line 466 [r1] (raw file):
I'm surprised you had AssemblyInfo files in compileFiles, that shouldn't be. AFAIK they exist in the project directories, and so are implicitly globbed by the compile default pattern.


Comments from the review on Reviewable.io

@vors
Copy link
Collaborator Author

vors commented Mar 23, 2016

Review status: all files reviewed at latest revision, 9 unresolved discussions.


mapping.json, line [r1] (raw file):
I don't like it eather, but the hope is it would not be around for the long time. I would keep it there


PowerShellGitHubDev.psm1, line 349 [r1] (raw file):
It's primarily for windows (Full PS developers). Json.NET is not a part of standard distro, so current version is more generic.


PowerShellGitHubDev.psm1, line 395 [r1] (raw file):
Done.


PowerShellGitHubDev.psm1, line 398 [r1] (raw file):
All this tools assume people know what they are doing.
I'm not a big fun built-in protections, that will affect perf (call to git is not cheap).


PowerShellGitHubDev.psm1, line 412 [r1] (raw file):
Yes, that would be the right place to do that.
Nothing rushes us to do encoding conversion right now, we can as well do it later to keep this step simpler.


PowerShellGitHubDev.psm1, line 436 [r1] (raw file):
Done.


PowerShellGitHubDev.psm1, line 441 [r1] (raw file):
No it's getting the mapping path pattern


PowerShellGitHubDev.psm1, line 450 [r1] (raw file):
It's not a skip, it's a different pattern.
These two projects will be skipped, because there are no files in compileFiles section.


PowerShellGitHubDev.psm1, line 466 [r1] (raw file):
There was one case like that.


Comments from the review on Reviewable.io

@andyleejordan
Copy link
Member

We'll need to add the resx files into the mapping too now.

@andyleejordan
Copy link
Member

Regarding #529, with the ps1xml files eliminated, all that's left to deploy from PowerShell are the default Modules. These actually make sense to be owned by the host instead of SMA, since each host may deploy different modules.

Does this support mapping the default Modules files into the superproject?

Sergei Vorobev added 5 commits March 29, 2016 16:56
There are 2 functions:

- New-MappingFile - creates mapping.json from project.json files
  (one is included in this commit)

- Copy-SubmoduleFiles - enables copying files back and forth from
  submodule to src/<projects> based on mapping.json file
This function will help us to port changes to source-depot
from server2016 branch
@vors vors force-pushed the vors/no-submodule branch from 58ff16c to afc1dd2 Compare March 30, 2016 21:06
@vors vors force-pushed the vors/no-submodule branch from afc1dd2 to 136e0ee Compare March 30, 2016 21:14
@vors vors merged commit 136e0ee into master Mar 31, 2016
@vors vors removed the in progress label Mar 31, 2016
@vors vors deleted the vors/no-submodule branch March 31, 2016 04:26
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.

3 participants