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

Delete and Move APIs #12

Open
moogar0880 opened this issue Mar 9, 2019 · 2 comments
Open

Delete and Move APIs #12

moogar0880 opened this issue Mar 9, 2019 · 2 comments
Labels
breaking change This issue or pull request introduces a breaking change enhancement New feature or request good first issue Good for newcomers

Comments

@moogar0880
Copy link
Owner

The purpose of this issue is to expand upon the existing ConfigStore API to add methods for removing and renaming keys in the ConfigStore.

API

Currently there is no mechanism for performing these operations within venom, however, the logic to implement them should be fairly simple. Two new methods should be added to the ConfigStore interface:

func Move(from, to string) error
func Delete(key string)

Both of these methods must be implemented by the Venom type, which is itself a ConfigStore, along with one additional method:

func MoveAndAlias(from, to string) error

This MoveAndAlias method will, just like it sounds, perform a Move and an Alias to ensure that the configuration data is available under the new keyspace, but that any applications using the old name will still be supported.

Additional Considerations

  • Delete operations should be considered successful even if the specified key does not exist
  • Move should return an error if either the from key does not exist or if data is already stored under the to keyspace.

Note: The addition of the Move and Delete methods to the ConfigStore API is a breaking change and will require a major version bump once merged.

@moogar0880 moogar0880 added enhancement New feature or request good first issue Good for newcomers breaking change This issue or pull request introduces a breaking change labels Mar 9, 2019
@ilmanzo
Copy link

ilmanzo commented Mar 13, 2019

that's an interesting idea. Should "Move" work across multiple levels ? e.g.

Move("foo.bar","foo") 
Move("foo","foo.bar")
Move("foo","foo.bar.baz")

in the latter case, should necessary intermediate sub-levels be created ?

@moogar0880
Copy link
Owner Author

Should "Move" work across multiple levels?

That's a good question, and I guess it depends what you mean by "levels". If "levels" of nested maps in the ConfigStore, then yeah I think so, otherwise you'd have to recursively move all of the keys under a root to truly move a config. For instance given a config that looks more or less like this:

{
    "foo": {
        "bar": {
            "baz": "https://example.com",
            "bat": "https://google.com"
        }
    }
}

A Move("foo.bar.baz", "foo") would result in

{
    "foo": "https://example.com"
}

However, if by "levels" you meant ConfigLevels in the heap, then that isn't something that'd I'd actually considered, but I think the answer is also yes. Which means we would effectively have to iterate over all of the config levels in the heap, check to see if the key to move has been set in that level or not and, if it has, then move it to the new key.

in the latter case, should necessary intermediate sub-levels be created ?

Yeah, I think they'd have to be, otherwise we wouldn't be able to traverse the config map properly to do a lookup or future inserts. Luckily setNested should handle that when the value is re-inserted under the new key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This issue or pull request introduces a breaking change enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants