Skip to content

push up stuff for PR#4

Open
jsmapr1 wants to merge 4 commits into
mainfrom
fake
Open

push up stuff for PR#4
jsmapr1 wants to merge 4 commits into
mainfrom
fake

Conversation

@jsmapr1
Copy link
Copy Markdown

@jsmapr1 jsmapr1 commented Feb 2, 2018

No description provided.

@jsmapr1 jsmapr1 requested a review from heaper February 2, 2018 21:46
@heaper
Copy link
Copy Markdown
Contributor

heaper commented Feb 2, 2018

Hey looks like there are some linting issues

Comment thread src/objectUtils.js Outdated
function alphabetizeKeys(target, blacklist = []) {
const usableKeys = [];
const keys = Object.keys(target);
for (var i = 0; i < keys.length; i++) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still see var statements

Comment thread src/objectUtils.js Outdated
function alphabetizeKeys(target, blacklist = []) {
const usableKeys = [];
const keys = Object.keys(target);
for (let i = 0; i < keys.length; i++) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you try filtering?

Comment thread src/objectUtils.js Outdated
const usableKeys = [];
const keys = Object.keys(target);
for (let i = 0; i < keys.length; i++) {
if (blacklist.indexOf(keys[i]) > -1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could be clarified with an Array.includes(), like so:

if (blacklist.includes(keys[i])) {

Base automatically changed from master to main January 26, 2021 15:59
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.

3 participants