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

forthcoming refactoring actions, following review of #368 #374

Open
4 of 8 tasks
fredbi opened this issue Jan 28, 2020 · 0 comments
Open
4 of 8 tasks

forthcoming refactoring actions, following review of #368 #374

fredbi opened this issue Jan 28, 2020 · 0 comments
Assignees

Comments

@fredbi
Copy link
Contributor

fredbi commented Jan 28, 2020

Reviewers comments from #368, left as TODOs, in a nutshell:

  • Confusing/pointless use of type alias for bool

My point was just to replace this by:
func MultiPut(ctx context.Context, stores []MultiStoreUnit, name string, buffer []byte, doesNotExist bool) error

  • separate porcelain funcs from cmd

refactor types defining such dsl-ish things to a separate package
Yes. Indeed this is the goal I am pursuing. I'd like many little things like this (including the whole flagsT, with default settings and GCS strategy) to move to some pkg/api/helpers or pkg/sdk/helpers package to be available as an higher level API for instance, a python wrapper. Please suggest a name for that.

  • func localPath(consumable fmt.Stringer) (string, error)

functions or methods defined in storage
remains a questionable design decision

  • cli_fuse_test.go: func testContext() string
    cruft?
  • I'd really like to move all this to some pkg/fuse package.

creating a package for the filesystem implementation stuff has been a todo for a while, although idk if we have an explicit issue for it.

  • Maybe the log level is best placed as a persistent flag under root?
  • CLI fuse_test: assert stdout & stderr separately, not through a combined output
  • clarify impact & priority of env var GOOGLE_APPLICATION_CREDENTIALS (re remark from Ransom)
@fredbi fredbi self-assigned this Jan 28, 2020
fredbi added a commit that referenced this issue Feb 1, 2020
Removed aliased type to bool.

Go aliased types (i.e. type A = B) are a syntactic sugar only.
This is different from type redefinitions (i.e. type A B), which create a new type for the compiler.

The original intent was to construct some kind of enumerated type, for a... boolean.

I chose to remove all this and just go back to plain bool.

* contributes #374

Signed-off-by: Frederic BIDON <[email protected]>
fredbi added a commit that referenced this issue Feb 1, 2020
* Moved up logLevel param to be at the root level.
* Added a "none" level to mute zap logging entirely

* contributes #374

Signed-off-by: Frederic BIDON <[email protected]>
fredbi added a commit that referenced this issue Feb 2, 2020
* Moved up logLevel param to be at the root level.
* Added a "none" level to mute zap logging entirely

* contributes #374

Signed-off-by: Frederic BIDON <[email protected]>

Co-authored-by: Ritesh H Shukla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant