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

Improve defaultChildNode #317

Merged
merged 3 commits into from
Jan 8, 2019

Conversation

parostatkiem-zz
Copy link
Contributor

Description

Changes proposed in this pull request:

  • Improve defaultChildNode function
  • add unit test

Related issue(s)

@parostatkiem-zz parostatkiem-zz added enhancement New feature or request area/luigi labels Jan 4, 2019
@parostatkiem-zz parostatkiem-zz added bug Something isn't working and removed enhancement New feature or request labels Jan 4, 2019
@y-kkamil y-kkamil self-assigned this Jan 7, 2019
@pekura pekura self-assigned this Jan 7, 2019
} else if (children && children.length > 0) {
return children[0].pathSegment;
} else if (children && children.length) {
const validChild = children.find(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not reviewing it, but saw it and would like to give feedback. I would rewrite is as

const validChild = children.find(
      child => child.pathSegment && (child.viewUrl || (child.externalLink && child.externalLink.url))
    );
    return validChild ? validChild.pathSegment : '';

Otherwise imo is like checking partially in the return statement if the validChild is valid, which doesn't make much sense

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 like it and I'll apply it 👍

@kwiatekus kwiatekus merged commit e590b92 into SAP:master Jan 8, 2019
@parostatkiem-zz parostatkiem-zz deleted the Improve-defaultChildNode-func branch February 5, 2019 08:27
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
Improve defaultChildNode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants