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

Show an error if auth type is incorrect #71

Closed
preslavmihaylov opened this issue Oct 13, 2020 · 6 comments
Closed

Show an error if auth type is incorrect #71

preslavmihaylov opened this issue Oct 13, 2020 · 6 comments
Labels
good first issue Good for newcomers UX

Comments

@preslavmihaylov
Copy link
Owner

preslavmihaylov commented Oct 13, 2020

Here's the error I get when the provided auth type in .todocheck.yaml is incorrect:
image

This happens because there is a check for it in one of the components using the config. It would be more appropriate to validate against this in the config.Validate function instead and show an error message to the user in a normal fashion, rather than relying on a panic, which happens afterwards.

@preslavmihaylov preslavmihaylov added good first issue Good for newcomers UX labels Oct 13, 2020
@akk5597
Copy link

akk5597 commented Oct 13, 2020

Would the solution be to call authmanager.AcquireToken in config.Validate, and replace the panic with an error saying that the auth token type is unknown?

Edit: That would not work as that would be an import cycle

@preslavmihaylov
Copy link
Owner Author

preslavmihaylov commented Oct 13, 2020

I think a good approach would be to make a function like this (taken from here):

func (lt LeaveType) IsValid() error {
    switch lt {
    case AnnualLeave, Sick, BankHoliday, Other:
        return nil
    }
    return errors.New("Invalid leave type")
}

This function should be in the same file as the enum declarations to avoid someone creating a new enum in the future without upgrading this func.

Using AcquireToken to validate is not a good idea, IMO, because it is not meant to be used for validation, but rather, for acquiring a token as the name states.

@zen37
Copy link
Contributor

zen37 commented Oct 14, 2020

It would be more appropriate to validate against this in the config.Validate function instead

Where is the config.Validate function? I could not find it.

@preslavmihaylov
Copy link
Owner Author

func (l *Local) Validate() []error {

@zen37
Copy link
Contributor

zen37 commented Oct 16, 2020

sorry, I didn't know of this convention, fileName.functionName and it is so logical.

zen37 pushed a commit to zen37/todocheck that referenced this issue Oct 18, 2020
zen37 pushed a commit to zen37/todocheck that referenced this issue Oct 25, 2020
preslavmihaylov pushed a commit that referenced this issue Oct 25, 2020
Co-authored-by: zen <zen@zen>
@preslavmihaylov
Copy link
Owner Author

Closed by #84

zen37 pushed a commit to zen37/todocheck that referenced this issue Oct 28, 2020
zen37 pushed a commit to zen37/todocheck that referenced this issue Oct 29, 2020
preslavmihaylov pushed a commit that referenced this issue Oct 29, 2020
* re-fixes issue #71 broken by PR #85

* re-fixes issue #71, removes validation of auth type from validation.go

Co-authored-by: zen <zen@zen>
Co-authored-by: Preslav Mihaylov <[email protected]>
preslavmihaylov pushed a commit that referenced this issue Oct 31, 2020
Co-authored-by: zen <zen@zen>
preslavmihaylov pushed a commit that referenced this issue Oct 31, 2020
* re-fixes issue #71 broken by PR #85

* re-fixes issue #71, removes validation of auth type from validation.go

Co-authored-by: zen <zen@zen>
Co-authored-by: Preslav Mihaylov <[email protected]>
mehdy pushed a commit that referenced this issue Nov 1, 2020
… code into separate components (#81)

* refactored issue tracker component, applying dependency inversion

* fix failing build for tests

* renamed all occurrencces of redmine to pivotaltracker in the pivotaltracker component

* fix issue #71 (#84)

Co-authored-by: zen <zen@zen>

* unblock issues caused by bad validation (#85)

* unblock issues caused by bad validation

* remove commented out code

* Enable annotating issues with hashtag (#86)

* Enable annotating github issues with hashtag

* add test

* revert ParseGithubDetails to parseGithubDetails

* revert README and auto_detect_config's change

* Add test for hashtag todos with github

* Add ExpectJSONOutput (#77)

* Add ExpectJSONOutput

* rename ExpectJsonOutput to WithJSONOutput, divide validateTodoErrs into two funcs

* modify the way to compare json todo errs and scenarios

* Add TestAnnotatedTodosWithJSONOutput

Co-authored-by: Preslav Mihaylov <[email protected]>

* re-fixes issue #71 broken by PR #85 (#87)

* re-fixes issue #71 broken by PR #85

* re-fixes issue #71, removes validation of auth type from validation.go

Co-authored-by: zen <zen@zen>
Co-authored-by: Preslav Mihaylov <[email protected]>

* refactored issue tracker component, applying dependency inversion

Co-authored-by: zen37 <[email protected]>
Co-authored-by: zen <zen@zen>
Co-authored-by: liao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers UX
Projects
None yet
Development

No branches or pull requests

3 participants