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

improvement(scully): add routes validation #136

Merged
merged 2 commits into from
Jan 4, 2020

Conversation

Villanuevand
Copy link
Contributor

When plugins generate routes for scully, we need to validate that the handledRoutes.route start with
/. In this change we show a warning.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?
Add validation to Handled Routes.

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

When plugins generate routes for scully, we need to validate that the handledRoutes.route start with
/. In this change we show a warning.
@Villanuevand Villanuevand added the enhancement New feature or request label Jan 3, 2020
@aaronfrost
Copy link
Contributor

I like this change, here is why:

  • We could have failed the build when we detect this. But since we haven't discussed it as a team, I like starting with the error and we can upgrade to a failed build if we decide that is better.
  • We could have just auto-added the / to the beginning of the route, but since we haven't talked about that approach as a team, I like adding a warning for now and we can always upgrade to this later if we decide to as a team.

I am approving this, but would like to see what @SanderElias says before we merge.

x.push({route: cur, type: RouteTypes.default});
}
return x;
}, Promise.resolve([] as HandledRoute[]));
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add a pre-commit hook that runs prettier on all files. This reformatting done on @Villanuevand's machine should have been done by whoever wrote this code.

We need to get everyone to turn on their prettier formatting in their IDE, and we need to get a pre-commit hook that formats your stuff for prettier.

Copy link
Contributor

@SanderElias SanderElias left a comment

Choose a reason for hiding this comment

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

LGTM

@Villanuevand Villanuevand merged commit 4aa6fb3 into master Jan 4, 2020
@Villanuevand Villanuevand deleted the refactor-add-optional-routes branch January 4, 2020 18:33
@Villanuevand
Copy link
Contributor Author

Villanuevand commented Jan 4, 2020

@aaronfrost
Issue #143 was created to solve the code format problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants