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

golangci-lint | string STRING has N occurrences, make it a constant (goconst) #176

Closed
atc0005 opened this issue Dec 12, 2019 · 1 comment · Fixed by #179
Closed

golangci-lint | string STRING has N occurrences, make it a constant (goconst) #176

atc0005 opened this issue Dec 12, 2019 · 1 comment · Fixed by #179
Assignees
Labels
bug Something isn't working builds CI linting
Milestone

Comments

@atc0005
Copy link
Owner

atc0005 commented Dec 12, 2019

Receiving these linting errors when building the branch intended to resolve #161:

config/config.go:413:15: string `NotSet` has 4 occurrences, make it a constant (goconst)
                c.AppName = "NotSet"
                            ^
config/config_test.go:54:14: string `.exe` has 3 occurrences, make it a constant (goconst)
                appName += ".exe"
                           ^
config/config_test.go:52:13: string `elbow` has 3 occurrences, make it a constant (goconst)
        appName := "elbow"
                   ^
config/get.go:166:10: string `text` has 4 occurrences, make it a constant (goconst)
                return "text"
                       ^
config/config_test.go:53:21: string `windows` has 3 occurrences, make it a constant (goconst)
        if runtime.GOOS == "windows" {
                           ^

As noted, there is a good bit of repetition between the test files, so goconst linting errors are thrown to indicate that. As of this writing I'm not sure of the best fix for this, aside from potentially refactoring to create and heavily use constants throughout the code base (which may not be a bad thing).

Spinning off this issue here in order to help keep the focus tighter on #161.

@atc0005 atc0005 added bug Something isn't working builds CI linting labels Dec 12, 2019
@atc0005 atc0005 added this to the v0.7.0 milestone Dec 12, 2019
@atc0005 atc0005 self-assigned this Dec 12, 2019
atc0005 added a commit that referenced this issue Dec 12, 2019
Added:

- Tests (partial implementation/coverage)
  - validate the most common code-paths (much more TODO)
    - config handling
    - logging setup
  - run tests as part of GitHub Actions Workflow
    - recursively
    - after getting deps, but before linting steps
      in order to allow early failure
  - update Makefile to run tests recursively, verbosely
- Fix configuration precedence handling
- Add string slice equality check from StackOverflow

Changed:

- Fix configuration precedence handling
  - Config file loses to everything except default config settings
- `config.logBuffer` moved to `logging.LogBuffer`
  - internal detail exposed for general use
    - may change this later
- Updated code (where apparent) to be more test friendly
  - e.g., use `io.Reader` instead of filename so that we can use
    an in-memory TOML config file for testing config parsing
    and precedence validation
- Split config package into smaller files in an effort to make
  related code easier to manage
- Fail if requested config file not found
  - previously an error was logged, but execution was allowed
    to continue.
- Move default values to Getter methods
  - use those methods instead of directly dereferencing config
    struct fields in the majority of the code
  - use Getter methods to guard against nil dereferences
- Partial work to de-couple reliance on `Config{}` (see #170)

Deprecated:

Both of these functions from the `config` package do not appear
to be needed any longer, but are being kept for a cycle in case
I change my mind:

- `Config.SetDefaultConfig()`
- `Config.GetStructTag()`

Fixed:

- Fix configuration precedence handling
  - Config settings are merged properly, so that even default
    settings are allowed to override lower precedence config
    sources
- NumFilesToKeep default (didn't match v0.5.1 change)
- Add missing `default` switch statements along with error
  return codes for `Set` functions with specific valid option values
- README updates
  - cover config file flag, environment variables
  - config precedence corrections
- Multiple linting errors (with more that needs to be evaluated)
- Add missing exit if IgnoreErrors is false
- Handle unintended nil assignment
- Update GoDoc doc file
  - reflect config file support, including updated Help text

TODO:

- Update multiple tests to use "tables" instead of hard-coding
  specific checks (which ends up being very lengthy)
  - e.g., `TestValidate()`
- Various goconst linting errors

refs #161, #156, #170, #176
atc0005 added a commit that referenced this issue Dec 12, 2019
Added:

- Tests (partial implementation/coverage)
  - validate the most common code-paths (much more TODO)
    - config handling
    - logging setup
  - run tests as part of GitHub Actions Workflow
    - recursively
    - after getting deps, but before linting steps
      in order to allow early failure
  - update Makefile to run tests recursively, verbosely
- Fix configuration precedence handling
- Add string slice equality check from StackOverflow

Changed:

- Fix configuration precedence handling
  - Config file loses to everything except default config settings
- `config.logBuffer` moved to `logging.LogBuffer`
  - internal detail exposed for general use
    - may change this later
- Updated code (where apparent) to be more test friendly
  - e.g., use `io.Reader` instead of filename so that we can use
    an in-memory TOML config file for testing config parsing
    and precedence validation
- Split config package into smaller files in an effort to make
  related code easier to manage
- Fail if requested config file not found
  - previously an error was logged, but execution was allowed
    to continue.
- Move default values to Getter methods
  - use those methods instead of directly dereferencing config
    struct fields in the majority of the code
  - use Getter methods to guard against nil dereferences
- Partial work to de-couple reliance on `Config{}` (see #170)

Deprecated:

Both of these functions from the `config` package do not appear
to be needed any longer, but are being kept for a cycle in case
I change my mind:

- `Config.SetDefaultConfig()`
- `Config.GetStructTag()`

Fixed:

- Fix configuration precedence handling
  - Config settings are merged properly, so that even default
    settings are allowed to override lower precedence config
    sources
- NumFilesToKeep default (didn't match v0.5.1 change)
- Add missing `default` switch statements along with error
  return codes for `Set` functions with specific valid option values
- README updates
  - cover config file flag, environment variables
  - config precedence corrections
- Multiple linting errors (with more that needs to be evaluated)
- Add missing exit if IgnoreErrors is false
- Handle unintended nil assignment
- Update GoDoc doc file
  - reflect config file support, including updated Help text

TODO:

- Update multiple tests to use "tables" instead of hard-coding
  specific checks (which ends up being very lengthy)
  - e.g., `TestValidate()`
- Various goconst linting errors

refs #161, #156, #170, #176
atc0005 added a commit that referenced this issue Dec 12, 2019
Added:

- Tests (partial implementation/coverage)
  - validate the most common code-paths (much more TODO)
    - config handling
    - logging setup
  - run tests as part of GitHub Actions Workflow
    - recursively
    - after getting deps, but before linting steps
      in order to allow early failure
  - update Makefile to run tests recursively, verbosely
- Fix configuration precedence handling
- Add string slice equality check from StackOverflow

Changed:

- Fix configuration precedence handling
  - Config file loses to everything except default config settings
- `config.logBuffer` moved to `logging.LogBuffer`
  - internal detail exposed for general use
    - may change this later
- Updated code (where apparent) to be more test friendly
  - e.g., use `io.Reader` instead of filename so that we can use
    an in-memory TOML config file for testing config parsing
    and precedence validation
- Split config package into smaller files in an effort to make
  related code easier to manage
- Fail if requested config file not found
  - previously an error was logged, but execution was allowed
    to continue.
- Move default values to Getter methods
  - use those methods instead of directly dereferencing config
    struct fields in the majority of the code
  - use Getter methods to guard against nil dereferences
- Partial work to de-couple reliance on `Config{}` (see #170)

Deprecated:

Both of these functions from the `config` package do not appear
to be needed any longer, but are being kept for a cycle in case
I change my mind:

- `Config.SetDefaultConfig()`
- `Config.GetStructTag()`

Fixed:

- Fix configuration precedence handling
  - Config settings are merged properly, so that even default
    settings are allowed to override lower precedence config
    sources
- NumFilesToKeep default (didn't match v0.5.1 change)
- Add missing `default` switch statements along with error
  return codes for `Set` functions with specific valid option values
- README updates
  - cover config file flag, environment variables
  - config precedence corrections
- Multiple linting errors (with more that needs to be evaluated)
- Add missing exit if IgnoreErrors is false
- Handle unintended nil assignment
- Update GoDoc doc file
  - reflect config file support, including updated Help text

TODO:

- Update multiple tests to use "tables" instead of hard-coding
  specific checks (which ends up being very lengthy)
  - e.g., `TestValidate()`
- Various goconst linting errors

refs #161, #156, #170, #176
atc0005 added a commit that referenced this issue Dec 12, 2019
Added:

- Tests (partial implementation/coverage)
  - validate the most common code-paths (much more TODO)
    - config handling
    - logging setup
  - run tests as part of GitHub Actions Workflow
    - recursively
    - after getting deps, but before linting steps
      in order to allow early failure
  - update Makefile to run tests recursively, verbosely
- Fix configuration precedence handling
- Add string slice equality check from StackOverflow

Changed:

- Fix configuration precedence handling
  - Config file loses to everything except default config settings
- `config.logBuffer` moved to `logging.LogBuffer`
  - internal detail exposed for general use
    - may change this later
- Updated code (where apparent) to be more test friendly
  - e.g., use `io.Reader` instead of filename so that we can use
    an in-memory TOML config file for testing config parsing
    and precedence validation
- Split config package into smaller files in an effort to make
  related code easier to manage
- Fail if requested config file not found
  - previously an error was logged, but execution was allowed
    to continue.
- Move default values to Getter methods
  - use those methods instead of directly dereferencing config
    struct fields in the majority of the code
  - use Getter methods to guard against nil dereferences
- Partial work to de-couple reliance on `Config{}` (see #170)

Deprecated:

Both of these functions from the `config` package do not appear
to be needed any longer, but are being kept for a cycle in case
I change my mind:

- `Config.SetDefaultConfig()`
- `Config.GetStructTag()`

Fixed:

- Fix configuration precedence handling
  - Config settings are merged properly, so that even default
    settings are allowed to override lower precedence config
    sources
- NumFilesToKeep default (didn't match v0.5.1 change)
- Add missing `default` switch statements along with error
  return codes for `Set` functions with specific valid option values
- README updates
  - cover config file flag, environment variables
  - config precedence corrections
- Multiple linting errors (with more that needs to be evaluated)
- Add missing exit if IgnoreErrors is false
- Handle unintended nil assignment
- Update GoDoc doc file
  - reflect config file support, including updated Help text

TODO:

- Update multiple tests to use "tables" instead of hard-coding
  specific checks (which ends up being very lengthy)
  - e.g., `TestValidate()`
- Various goconst linting errors

refs #161, #156, #170, #176
atc0005 added a commit that referenced this issue Dec 12, 2019
Added:

- Tests (partial implementation/coverage)
  - validate the most common code-paths (much more TODO)
    - config handling
    - logging setup
  - run tests as part of GitHub Actions Workflow
    - recursively
    - after getting deps, but before linting steps
      in order to allow early failure
  - update Makefile to run tests recursively, verbosely
- Fix configuration precedence handling
- Add string slice equality check from StackOverflow

Changed:

- Fix configuration precedence handling
  - Config file loses to everything except default config settings
- `config.logBuffer` moved to `logging.LogBuffer`
  - internal detail exposed for general use
    - may change this later
- Updated code (where apparent) to be more test friendly
  - e.g., use `io.Reader` instead of filename so that we can use
    an in-memory TOML config file for testing config parsing
    and precedence validation
- Split config package into smaller files in an effort to make
  related code easier to manage
- Fail if requested config file not found
  - previously an error was logged, but execution was allowed
    to continue.
- Move default values to Getter methods
  - use those methods instead of directly dereferencing config
    struct fields in the majority of the code
  - use Getter methods to guard against nil dereferences
- Partial work to de-couple reliance on `Config{}` (see #170)

Deprecated:

Both of these functions from the `config` package do not appear
to be needed any longer, but are being kept for a cycle in case
I change my mind:

- `Config.SetDefaultConfig()`
- `Config.GetStructTag()`

Fixed:

- Fix configuration precedence handling
  - Config settings are merged properly, so that even default
    settings are allowed to override lower precedence config
    sources
- NumFilesToKeep default (didn't match v0.5.1 change)
- Add missing `default` switch statements along with error
  return codes for `Set` functions with specific valid option values
- README updates
  - cover config file flag, environment variables
  - config precedence corrections
- Multiple linting errors (with more that needs to be evaluated)
- Add missing exit if IgnoreErrors is false
- Handle unintended nil assignment
- Update GoDoc doc file
  - reflect config file support, including updated Help text

TODO:

- Update multiple tests to use "tables" instead of hard-coding
  specific checks (which ends up being very lengthy)
  - e.g., `TestValidate()`
- Various goconst linting errors

refs #161, #156, #170, #176
atc0005 added a commit that referenced this issue Dec 12, 2019
Added:

- Tests (partial implementation/coverage)
  - validate the most common code-paths (much more TODO)
    - config handling
    - logging setup
  - run tests as part of GitHub Actions Workflow
    - recursively
    - after getting deps, but before linting steps
      in order to allow early failure
  - update Makefile to run tests recursively, verbosely
- Fix configuration precedence handling
- Add string slice equality check from StackOverflow

Changed:

- Fix configuration precedence handling
  - Config file loses to everything except default config settings
- `config.logBuffer` moved to `logging.LogBuffer`
  - internal detail exposed for general use
    - may change this later
- Updated code (where apparent) to be more test friendly
  - e.g., use `io.Reader` instead of filename so that we can use
    an in-memory TOML config file for testing config parsing
    and precedence validation
- Split config package into smaller files in an effort to make
  related code easier to manage
- Fail if requested config file not found
  - previously an error was logged, but execution was allowed
    to continue.
- Move default values to Getter methods
  - use those methods instead of directly dereferencing config
    struct fields in the majority of the code
  - use Getter methods to guard against nil dereferences
- Partial work to de-couple reliance on `Config{}` (see #170)

Deprecated:

Both of these functions from the `config` package do not appear
to be needed any longer, but are being kept for a cycle in case
I change my mind:

- `Config.SetDefaultConfig()`
- `Config.GetStructTag()`

Fixed:

- Fix configuration precedence handling
  - Config settings are merged properly, so that even default
    settings are allowed to override lower precedence config
    sources
- NumFilesToKeep default (didn't match v0.5.1 change)
- Add missing `default` switch statements along with error
  return codes for `Set` functions with specific valid option values
- README updates
  - cover config file flag, environment variables
  - config precedence corrections
- Multiple linting errors (with more that needs to be evaluated)
- Add missing exit if IgnoreErrors is false
- Handle unintended nil assignment
- Update GoDoc doc file
  - reflect config file support, including updated Help text

TODO:

- Update multiple tests to use "tables" instead of hard-coding
  specific checks (which ends up being very lengthy)
  - e.g., `TestValidate()`
- Various goconst linting errors

refs #161, #156, #170, #176
@atc0005
Copy link
Owner Author

atc0005 commented Dec 12, 2019

Research: How common is it to create a package specific to constants used broadly throughout the code base? Or, how common is it to duplicate constants between packages in order to isolate those package dependencies?

For example, if I want to create a constant to provide text as a valid logging format option, should I create:

  • config.FormatText
  • logging.FormatText

or use some other combination? If I already have a dependency between logging and config, perhaps create the constants in logging and reference them as needed elsewhere?

atc0005 added a commit that referenced this issue Dec 12, 2019
- Create minimum (or near about) constants to resolve
  golangci-lint's goconst linting tests

- Drop a good many TODO markers for further refactoring work

This is meant to get the CI builds passing again and clearly
note places that need further refinement for near-future
focus work.

refs #176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builds CI linting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant