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

Validate origin format based on issue tracker #47

Merged
merged 14 commits into from
Sep 21, 2020

Conversation

srafi1
Copy link
Contributor

@srafi1 srafi1 commented Aug 27, 2020

Solves #25

@srafi1
Copy link
Contributor Author

srafi1 commented Aug 27, 2020

I'm not sure how to validate the origin for Jira or Redmine, but this should validate the others properly

Copy link
Owner

@preslavmihaylov preslavmihaylov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to put together this PR!

Left some comments & suggestions for validating Jira and redmine.

I can help with testing the new validations for all supported issue trackers once you're ready.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@srafi1 srafi1 marked this pull request as ready for review August 29, 2020 18:46
@srafi1
Copy link
Contributor Author

srafi1 commented Aug 29, 2020

Thanks for the comments. I fixed everything you mentioned, but I have a couple of questions. Why does Validate() return a list of errors when it can only ever have one according to the current logic? Also, when I ran the tests with go test ./testing -v, many of the tests failed. I ended up running my own tests using go test ./testing -run Origins -v and although they pass, I'm not sure if I'm running tests properly if the other ones are failing on my computer.

@preslavmihaylov preslavmihaylov self-requested a review September 4, 2020 05:27
Copy link
Owner

@preslavmihaylov preslavmihaylov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few more comments left to make this fully compatible with what's currently supported & for adding more extensive test cases.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
origin: https://github.com/user/project
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add valid configs for all supported issue trackers:

  • Without a scheme provided - e.g. github.com/user/project
  • With www provided - e.g. www.github.com/user/project and https://www.github.com/user/project

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in defa509

testing/todocheck_test.go Show resolved Hide resolved
@preslavmihaylov
Copy link
Owner

Thanks for the comments. I fixed everything you mentioned, but I have a couple of questions. Why does Validate() return a list of errors when it can only ever have one according to the current logic? Also, when I ran the tests with go test ./testing -v, many of the tests failed. I ended up running my own tests using go test ./testing -run Origins -v and although they pass, I'm not sure if I'm running tests properly if the other ones are failing on my computer.

The tests all pass when I run them from latest master:
image

Could you try running the test suite from latest master & see if they pass?

As for the Validate function, the goal is to batch report all issues with the configuration.

Currently, your validation is incompatible with the rest but as more validations are added, there will be independent issues \w configurations which can be reported to the user.

config/config.go Outdated
@@ -82,6 +90,14 @@ func (l *Local) Validate() []error {
errors = append(errors, err)
}

if 0 == len(errors) {
Copy link
Owner

@preslavmihaylov preslavmihaylov Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is dependent on the order of validations. This shouldn't happen as this piece of validation might as well become first in line in the future.

Instead, change your condition to be independent from the errors array by validating that the issue tracker is not empty:

if l.IssueTracker != "" {
   ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in f4977d0

@preslavmihaylov
Copy link
Owner

Blocked by #54

@preslavmihaylov
Copy link
Owner

Hey, the PR blocking this one is now merged. Please pull latest master & continue from there (to enable the CI checks).

@srafi1
Copy link
Contributor Author

srafi1 commented Sep 15, 2020

I figured out the testing issue on my end, added a bunch of tests for the regex, and fixed the regex according to your comments. LMK if there's anything else to add.

Copy link
Owner

@preslavmihaylov preslavmihaylov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good this time around in terms of implementation, thanks!

I only have a single comment for the err message format used to report issues with invalid origins.

config/config.go Outdated Show resolved Hide resolved
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.

2 participants