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

clean code #2289

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/execute/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import getIn from 'lodash/get';
import get from 'lodash/get';
import isPlainObject from 'lodash/isPlainObject';
import url from 'url';
import cookie from 'cookie';
Expand All @@ -21,22 +21,23 @@ const OperationNotFoundError = createError(
}
);

const findParametersWithName = (name, parameters) => parameters.filter((p) => p.name === name);
const findParametersWithName = (name, parameters) =>
parameters.filter((parameter) => parameter.name === name);

// removes parameters that have duplicate 'in' and 'name' properties
const deduplicateParameters = (parameters) => {
const paramsMap = {};
parameters.forEach((p) => {
if (!paramsMap[p.in]) {
paramsMap[p.in] = {};
parameters.forEach((parameter) => {
if (!paramsMap[parameter.in]) {
paramsMap[parameter.in] = {};
}
paramsMap[p.in][p.name] = p;
paramsMap[parameter.in][parameter.name] = parameter;
});

const dedupedParameters = [];
Object.keys(paramsMap).forEach((i) => {
Object.keys(paramsMap[i]).forEach((p) => {
dedupedParameters.push(paramsMap[i][p]);
Object.keys(paramsMap).forEach((param) => {
Object.keys(paramsMap[param]).forEach((parameter) => {
dedupedParameters.push(paramsMap[param][parameter]);
});
});
return dedupedParameters;
Expand Down Expand Up @@ -285,9 +286,9 @@ export function baseUrl(obj) {

function oas3BaseUrl({ spec, pathName, method, server, contextUrl, serverVariables = {} }) {
const servers =
getIn(spec, ['paths', pathName, (method || '').toLowerCase(), 'servers']) ||
getIn(spec, ['paths', pathName, 'servers']) ||
getIn(spec, ['servers']);
get(spec, ['paths', pathName, (method || '').toLowerCase(), 'servers']) ||
get(spec, ['paths', pathName, 'servers']) ||
get(spec, ['servers']);

let selectedServerUrl = '';
let selectedServerObj = null;
Expand Down
2 changes: 2 additions & 0 deletions src/execute/oas3/style-serializer.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { Buffer } from 'buffer';
Copy link
Member

Choose a reason for hiding this comment

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

Why we added import to Buffer here? We got rid of it in e96702f


const isRfc3986Reserved = (char) => ":/?#[]@!$&'()*+,;=".indexOf(char) > -1;
const isRrc3986Unreserved = (char) => /^[a-z0-9\-._~]+$/i.test(char);

Expand Down
10 changes: 6 additions & 4 deletions src/execute/swagger2/build-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,22 @@ export default function buildRequest(options, req) {
[req.headers['Content-Type']] = spec.consumes;
} else if (
operation.parameters &&
operation.parameters.filter((p) => p.type === 'file').length
operation.parameters.filter((parameter) => parameter.type === 'file').length
) {
req.headers['Content-Type'] = 'multipart/form-data';
} else if (
operation.parameters &&
operation.parameters.filter((p) => p.in === 'formData').length
operation.parameters.filter((parameter) => parameter.in === 'formData').length
) {
req.headers['Content-Type'] = 'application/x-www-form-urlencoded';
}
} else if (requestContentType) {
const isBodyParamPresent =
operation.parameters && operation.parameters.filter((p) => p.in === 'body').length > 0;
operation.parameters &&
operation.parameters.filter((parameter) => parameter.in === 'body').length > 0;
const isFormDataParamPresent =
operation.parameters && operation.parameters.filter((p) => p.in === 'formData').length > 0;
operation.parameters &&
operation.parameters.filter((parameter) => parameter.in === 'formData').length > 0;
if (isBodyParamPresent || isFormDataParamPresent) {
req.headers['Content-Type'] = requestContentType;
}
Expand Down
6 changes: 3 additions & 3 deletions src/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,10 @@ export function normalizeSwagger(parsedSpec) {

const opList = map[oid];
if (opList.length > 1) {
opList.forEach((o, i) => {
opList.forEach((op, key) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why key here and not index? key is usually used in object context, not in array context

// eslint-disable-next-line no-underscore-dangle
o.__originalOperationId = o.__originalOperationId || o.operationId;
o.operationId = `${oid}${i + 1}`;
op.__originalOperationId = op.__originalOperationId || op.operationId;
op.operationId = `${oid}${key + 1}`;
});
} else if (typeof operation.operationId !== 'undefined') {
// Ensure we always add the normalized operation ID if one already exists
Expand Down
12 changes: 7 additions & 5 deletions src/http/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ function formatKeyValue(key, input, skipEncoding = false) {
const { collectionFormat, allowEmptyValue, serializationOption, encoding } = input;
// `input` can be string
const value = typeof input === 'object' && !Array.isArray(input) ? input.value : input;
const encodeFn = skipEncoding ? (k) => k.toString() : (k) => encodeURIComponent(k);
const encodeFn = (k) => (skipEncoding ? k.toString() : encodeURIComponent(k));
Copy link
Member

@char0n char0n Oct 19, 2021

Choose a reason for hiding this comment

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

I'm wondering here what is the motivation and arguments behind your change? If I would to simplify this I'd probably go with the following:

Suggested change
const encodeFn = (k) => (skipEncoding ? k.toString() : encodeURIComponent(k));
const encodeFn = skipEncoding ? String : encodeURIComponent;

Explanation: it's a simple ternary that chooses an appropriate function to be used as value of encodeFn

const encodedKey = encodeFn(key);

if (typeof value === 'undefined' && allowEmptyValue) {
Expand Down Expand Up @@ -290,8 +290,10 @@ function formatKeyValueBySerializationOption(key, value, skipEncoding, serializa
: serializationOption && serializationOption.allowReserved
? 'unsafe'
: 'reserved';
const encodeFn = (v) => encodeDisallowedCharacters(v, { escape });
const encodeKeyFn = skipEncoding ? (k) => k : (k) => encodeDisallowedCharacters(k, { escape });
const encodeFn = (str) => encodeDisallowedCharacters(str, { escape });
Copy link
Member

@char0n char0n Oct 19, 2021

Choose a reason for hiding this comment

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

Using str as variable name is consistent with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent, so let's go for it.

If our goal is simplifying the cleaning the code what would you say to following?

Suggested change
const encodeFn = (str) => encodeDisallowedCharacters(str, { escape });
const identity = (v) => v;
const encodeFn = (str) => encodeDisallowedCharacters(str, { escape });
const encodeKeyFn = skipEncoding ? identity : encodeFn;

const encodeKeyFn = skipEncoding
? (str) => str
: (str) => encodeDisallowedCharacters(str, { escape });

// Primitive
if (typeof value !== 'object') {
Expand Down Expand Up @@ -432,8 +434,8 @@ export function mergeInQueryOrForm(req = {}) {

// Wrap a http function ( there are otherways to do this, consider this deprecated )
export function makeHttp(httpFn, preFetch, postFetch) {
postFetch = postFetch || ((a) => a);
preFetch = preFetch || ((a) => a);
postFetch = postFetch || ((req) => req);
Copy link
Member

Choose a reason for hiding this comment

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

Regarding variable naming - yeah, great change!

(req) => req is an identity function. We already needed one in https://github.com/swagger-api/swagger-js/pull/2289/files#r731893085. Let's define an identity function in top of the file and just use it in my previous comment suggestion and in here.

preFetch = preFetch || ((req) => req);
return (req) => {
if (typeof req === 'string') {
req = { url: req };
Expand Down
4 changes: 2 additions & 2 deletions src/specmap/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class SpecMap {
return true;
}

return path.every((val, i) => val === tested[i]);
return path.every((val, key) => val === tested[key]);
Copy link
Member

Choose a reason for hiding this comment

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

key is associated with objects. Arrays use index naming.

};

return function* generator(patches, specmap) {
Expand Down Expand Up @@ -342,7 +342,7 @@ class SpecMap {

// A different plugin runs, wait for all promises to resolve, then retry
if (plugin !== this.currentPlugin && this.promisedPatches.length) {
const promises = this.promisedPatches.map((p) => p.value);
const promises = this.promisedPatches.map((parameter) => parameter.value);
Copy link
Member

Choose a reason for hiding this comment

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

origina p represent a patch here and not parameter, or am I wrong?

Suggested change
const promises = this.promisedPatches.map((parameter) => parameter.value);
const promises = this.promisedPatches.map((patch) => patch.value);


// Waits for all to settle instead of Promise.all which stops on rejection
return Promise.all(promises.map((promise) => promise.then(noop, noop))).then(() =>
Expand Down
4 changes: 2 additions & 2 deletions src/specmap/lib/all-of.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export default {
// remove existing content
patches.push(specmap.replace(parent, {}));

val.forEach((toMerge, i) => {
val.forEach((toMerge, position) => {
if (!specmap.isObject(toMerge)) {
if (alreadyAddError) {
return null;
Expand All @@ -68,7 +68,7 @@ export default {

const absoluteRefPatches = generateAbsoluteRefPatches(toMerge, collapsedFullPath, {
getBaseUrlForNodePath: (nodePath) =>
specmap.getContext([...fullPath, i, ...nodePath]).baseDoc,
specmap.getContext([...fullPath, position, ...nodePath]).baseDoc,
specmap,
});

Expand Down
4 changes: 2 additions & 2 deletions src/specmap/lib/context-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ export default class ContextTree {
let branch = this.root;
let child;
let token;
for (let i = 0; i < path.length; i += 1) {
token = path[i];
for (let position = 0; position < path.length; position += 1) {
token = path[position];
child = branch.children;
if (!child[token]) {
break;
Expand Down
4 changes: 2 additions & 2 deletions src/specmap/lib/parameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ export default {
const opPath = fullPath.slice(0, -1);
const op = { ...lib.getIn(specmap.spec, opPath) };

parameters.forEach((param, i) => {
parameters.forEach((param, position) => {
try {
val[i].default = specmap.parameterMacro(op, param);
val[position].default = specmap.parameterMacro(op, param);
} catch (e) {
const err = new Error(e);
err.fullPath = fullPath;
Expand Down
4 changes: 2 additions & 2 deletions src/specmap/lib/properties.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ export default {
const val = { ...properties };

// eslint-disable-next-line no-restricted-syntax, guard-for-in
for (const k in properties) {
for (const position in properties) {
Copy link
Member

Choose a reason for hiding this comment

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

How about changing k to key here? It actually says what the variable is in context of for in on object.

try {
val[k].default = specmap.modelPropertyMacro(val[k]);
val[position].default = specmap.modelPropertyMacro(val[position]);
} catch (e) {
const err = new Error(e);
err.fullPath = fullPath; // This is an array
Expand Down
10 changes: 5 additions & 5 deletions src/specmap/lib/refs.js
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,8 @@ function extractFromDoc(docPath, pointer) {
// `spec` and `docCache[basePath]` refer to the exact same object).
// See test "should resolve a cyclic spec when baseDoc is specified".
try {
const v = extract(pointer, doc);
return Object.assign(Promise.resolve(v), { __value: v });
const val = extract(pointer, doc);
return Object.assign(Promise.resolve(val), { __value: val });
} catch (e) {
return Promise.reject(e);
}
Expand Down Expand Up @@ -544,9 +544,9 @@ function pointerAlreadyInPath(pointer, basePath, parent, specmap) {
*/
function patchValueAlreadyInPath(root, patch) {
const ancestors = [root];
patch.path.reduce((parent, p) => {
ancestors.push(parent[p]);
return parent[p];
patch.path.reduce((parent, currentValue) => {
ancestors.push(parent[currentValue]);
return parent[currentValue];
}, root);
return pointToAncestor(patch.value);

Expand Down