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

Move validations into the arg class #703

Open
3 tasks
ArinGhazarian opened this issue Oct 26, 2022 · 0 comments
Open
3 tasks

Move validations into the arg class #703

ArinGhazarian opened this issue Oct 26, 2022 · 0 comments

Comments

@ArinGhazarian
Copy link
Collaborator

ArinGhazarian commented Oct 26, 2022

Currently we perform the arg validations inside the command handler class sometimes right inside the handle method or in its own dedicated method (or more than one method even).

The problems with this approach are:

  1. We do the validations after we create the handler which is not ideal. We shouldn't even create the handler if the validation fails.
  2. There is no uniform pattern for validations. Sometimes we do them in a single place or sometimes they are scattered in a handler class inside different methods which makes it harder to keep a track and make changes, also there is a possibility of missing some.

Todo

  • Create a Validate method inside each arg class (it can come from a base arg class and be overridden in the arg class) and put all arg validations in it.
  • Remove the validations from the command handler classes and call the Validate method in the command class as part of the BuildHandler method.
  • Rethink about arg validations case by case specially for larger commands like migrate-repo or generate-script. Like for example in bb2gh migrate-repo command if we don't pass the --github-org and --github-repo our code is not going to work properly, so we need to reconsider all possible scenarios and write appropriate tests to cover them.
dylan-smith added a commit that referenced this issue Jan 12, 2023
<!--
For the checkboxes below you must check each one to indicate that you
either did the relevant task, or considered it and decided there was
nothing that needed doing
-->

This refactor is necessary for some of the other refactoring being done
for #703

- Consolidated the 3x `EnvironmentVariableProvider` classes into one
- For `ado2gh` changed it to use `TargetGithubPersonalAccessToken`
instead of the previously named `GithubPersonalAccessToken`
- Changed all functions to accept a `throwIfNotFound` parameter
- Updated all test mocks to have `It.IsAny<bool>` for the
`throwIfNotFound` arg

## Checklist
- [x] Did you write/update appropriate tests
- [x] Release notes updated (if appropriate)
- [x] Appropriate logging output
- [x] Issue linked
- [x] Docs updated (or issue created)

<!--
For docs we should review the docs at:

https://docs.github.com/en/early-access/github/migrating-with-github-enterprise-importer
and the README.md in this repo

If a doc update is required based on the changes in this PR, it is
sufficient to create an issue and link to it here. The doc update can be
made later/separately.

The process to update the docs can be found here:
https://github.com/github/docs-early-access#opening-prs

The markdown files are here: 

https://github.com/github/docs-early-access/tree/main/content/github/migrating-with-github-enterprise-importer
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant