Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

Conversation

@marcofriso
Copy link
Contributor

Closes APIARY-6482

@marcofriso marcofriso requested a review from a team April 22, 2020 15:45

// if you define a variable that is not in URL it warns (and the variable is discarded).
variablesKeys.forEach((key) => {
if (urlVariables.indexOf(key) === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (urlVariables.indexOf(key) === -1) {
if (!urlVariables.includes(key)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

variablesKeys.forEach((key) => {
if (urlVariables.indexOf(key) === -1) {
parseResult.push(createWarning(context.namespace,
`Server variable '${key}' is not present in the URL and will be discarted`, variables));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`Server variable '${key}' is not present in the URL and will be discarted`, variables));
`Server variable '${key}' is not present in the URL and will be ignored`, variables));

discarted has a typo, but instead I think "ignored" sounds a little better here. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can put ignored, however, as the variable is taken away, discarded may make the idea clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anyway it has been replaced

const name = 'Server Object';
const requiredKeys = ['url'];

const validateVariableInURL = (context, object) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const validateVariableInURL = (context, object) => {
const validateVariablesInURL = (context, object) => {

I'd make variable plural in the method name as we're validating a set of variables not singular variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good, change on next commit

.map(x => x.replace(/[{}]/g, ''));

// if you define a variable that is not in URL it warns (and the variable is discarded).
variablesKeys.forEach((key) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
variablesKeys.forEach((key) => {
variables.keys().forEach((key) => {

There's a method for this already so you can delete the above const variablesKeys = variables.content.map(variable => variable.key.toValue());.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, changed

urlVariables.forEach((key) => {
if (!variables.hasKey(key)) {
parseResult.push(createWarning(context.namespace,
`URL variable '${key}' is missing within the server variables`, variables));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`URL variable '${key}' is missing within the server variables`, variables));
`URL variable '${key}' is missing within the server variables`, object.get('url')));

I think we want to use the URL here to create the warning, because the source map for the warning should originate from the URL which contains the variable which isn't defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see, changed

@marcofriso marcofriso requested a review from a team April 23, 2020 14:31
@marcofriso marcofriso force-pushed the marco/server-variables-validation branch from 4dccd1c to a39f634 Compare April 27, 2020 08:40
@marcofriso
Copy link
Contributor Author

@kylef merge?

@kylef kylef merged commit 2dcde2d into master Apr 27, 2020
@kylef kylef deleted the marco/server-variables-validation branch April 27, 2020 15:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants