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

patch(persist): Persistence with type [Object] and [Array] #3248

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Mar 21, 2019

Totally rewrite using PowerShell's Verb-Noun.

Reference: #2897 (comment) #3209 #3212

Refactor persistence implementation, by which maintainers could use [Object] or [Array](for backward compatible) type of persist items to persist directories or files.

persist Demos

(can also be found in test\fixtures\persist)

  • [Object] type persistence
{
    "persist": [
        {
            "name": "foo",
            "type": "directory"
        }, {
            "name": "foo\\",
            "target": "bar"
        }, {
            "name": "foo",
            "type": "folder",
            "method": "merge"
        }, {
            "name": "foo",
            "type": "file"
        }, {
            "name": "foo",
            "contents": [
                "file",
                "contents"
            ],
            "glue": " ",
            "encoding": "UTF8",
            "method": "update"
        }
    ]
}
  • [Array] type persistence
{
    "persist": [
        "foo\\bar",
        [
            "foo",
            "bar"
        ],
        [
            "foo"
        ],
        [
            "foo",
            "",
            "file",
            "contents"
        ]
    ]
}

Properties Defination

  • [Object] (forked from [WIP] Persistence rework with backward compatibility (persist) #3212)
    • name: file or directory name (required)
    • type: either file or directory (alias: folder, dir) (optional)
      • file by default, directory if name ends with \\ or / (see demos)
    • target: persisted name in $persist_dir (optional)
    • method (optional)
      • link: create hardlink for files or junctions for directories (default)
        • if existed in $persist_dir, link it; otherwise, copy to $persist_dir and link it
      • copy: just copy directory or file
        • no matter existed in $persist_dir, copy origin to $persist_dir and link it
      • merge: merge new persistent directory (Languages/Plugins/Skins/etc.) with current persistent directory
        • only copy new contents to current persistence (i.e., that not existed in persistent dir)
      • update: update current persistent directory with new persistent directory
        • copy new contents and newer contents to current persistence(i.e., that not existed in persistent dir and that with newer modification time)
    • encoding: file encoding (optional, default ASCII, not used for directories)
      • enum: ASCII, UTF8, OEM, etc.
    • content: either string or array of strings (optional, not used for directories)
      • support internal variables (e.g., $persist_dir, $architecture, etc.)
    • glue: characters to join content array (optional, default \r\n, not used for directories)
  • [Array] (forked from fix(persist): Using [Array] to persist file  #3209)
    • Count: [Array].Count is used to determine if it is file or directory
      • Count = 1: item is file, and file content is empty string ("", not $null)
      • Count = 2: item is dir, and the second element is the persisted target name
      • Count >= 3: item is file, the second element is the persisted target name (can be "" or $null if not changed), and the remaining elements is file contents which will be joined with \r\n

Implementation

  • Seperate original persist_data into three part:
    • Core helper function: PersistentHelper($Source, $Target, $Content, $Method, $Encoding)
      • Actually do the persistence
    • Defination parser function: Get-PersistentDefination($Persist)
      • Get persisting definations from $Persist, use different mothods according to $Persist's type ([PSCustomObject], [Object[]] or [String])
    • Wrapper function: Add-PersistentLink($Persist, $InstalledPath, $PersistentPath)
      • Use parser function to get persisting defination, then call core function to persist items

Test Cases and Examples

  • Scoop-Install.Tests.ps1: testing parsing $persist, with item type [Object] and [Array]
  • For practical examples, please check nvm-object.json and nvm-array.json attached

Please test and review, thanks.
@r15ch13 @rasa @h404bi @Ash258

@Ash258
Copy link
Contributor

Ash258 commented Mar 21, 2019

I don't like that array syntax. It's mess and not much readable, manifests should be straightforward and easy to maintain. For shortcuts it's OK, but here it's not much something you realy need and want to maintain (Mainly there has to be duplication in folder definition (if you want to keep same name), which i generaly really try to avoid in manifests).

So i am unsubscribing from this PR and somone else might review it. I will strict only to #3212. #3212 will make persists more than suitable.

@niheaven
Copy link
Member Author

You can just use new [Object] type as defined in #3212, which is also implemented here, and [Array] type is for backward compatible, tweaked with some improvement to allow persisting file with it alone. These are not mutually exclusive, I think.

@Ash258
Copy link
Contributor

Ash258 commented Mar 21, 2019

Yes. I know I can. But i just don't like it and for me it's waste of time to review this when I can review just code, which i like and which i believe is better for maintaining. When collaborators decide, this will be merged instead of #3212 (I hope this never happen), then why not, but i will still don't see a reason to invest my time into reviewing this PR.

I just wanted to clarify 2 things:

  1. My personal negative opinion about this change
  2. Notify contributors to not mention me here for any reason

@niheaven
Copy link
Member Author

Okay, if after discussion, [Array] implementation is really messy and useless, I could rebase the commits to drop it and just keep [Object] and backward compatible.

Anyone else could review and test any parts of this PR, this two implementations use the same core function and wrapper function, with different persist_def helper function.

@niheaven niheaven force-pushed the persist-file branch 2 times, most recently from 34fcba6 to 667e773 Compare March 28, 2019 03:56
@niheaven niheaven changed the base branch from master to develop April 25, 2019 09:54
@niheaven niheaven force-pushed the persist-file branch 2 times, most recently from d633752 to 7e5da35 Compare May 17, 2019 10:44
kidonng referenced this pull request in chawyehsu/dorado Jul 22, 2019
@niheaven
Copy link
Member Author

test/Scoop-Core.Tests.ps1 passed locally

Error code 16 of robocopy is:

0×10  16       Serious error. Robocopy did not copy any files.
               Either a usage error or an error due to insufficient access privileges
               on the source or destination directories.

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