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

Imported only lodash submodules #2033

Closed
wants to merge 3 commits into from
Closed

Conversation

MeenuyD
Copy link
Contributor

@MeenuyD MeenuyD commented Dec 7, 2023

Imported only lodash submodules
#2032

@MeenuyD MeenuyD requested a review from a team as a code owner December 7, 2023 11:52
@MeenuyD MeenuyD requested review from albertteoh and removed request for a team December 7, 2023 11:52
Comment on lines 44 to 49
const tags = _(availableTags).map('tags').flatten().value();
let tagKeys = _(tags).map('key').uniq().value();
tagKeys = _.filter(tagKeys, function calc(o) {
const tags = _map(availableTags).map('tags').flatten().value();
let tagKeys = _map(tags).map('key').uniq().value();
tagKeys = _map.filter(tagKeys, function calc(o) {
Copy link
Member

@anshgoyalevil anshgoyalevil Dec 7, 2023

Choose a reason for hiding this comment

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

We aren't only using the _map submodule here. Others are also used like _uniq, _flatter, _concat.

A changed syntax would be something like:

  const tags = _flatten(_map(availableTags, 'tags'));
  let tagKeys = _uniq(_map(tags, 'key'));
...etc

Signed-off-by: MeenuyD <meenu.coninja@gmail.com>
Signed-off-by: MeenuyD <meenu.coninja@gmail.com>
@yurishkuro
Copy link
Member

It will go faster if you run yarn lint/test locally before submitting

Comment on lines +47 to +49
const tags = _flatten(availableTags).map('tags').flatten().value();
let tagKeys = _uniq(tags).map('key').uniq().value();
tagKeys = _map.filter(tagKeys, function calc(o) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you have messed up the whole thing here. I think you should take the reference of official lodash docs for better understanding.

Comment on lines +83 to +85
const tags = _flatten(allSpans).map('tags').flatten().value();
const tagKeys = _uniq(tags).map('key').uniq().value();
const values = _concat.concat(serviceName, operationName, tagKeys);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

const temp = _.chain(allSpans)
const temp = _get.chain(allSpans)
Copy link
Member

Choose a reason for hiding this comment

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

chain is not a function provided by _get, rather is a submodule itself. Please check the comments above ^

@yurishkuro yurishkuro added the changelog:dependencies Update to dependencies label Dec 8, 2023
@yurishkuro
Copy link
Member

I moved to files that were changed correctly into another PR, already merged.

@MeenuyD
Copy link
Contributor Author

MeenuyD commented Dec 9, 2023

Ok thank you

@yurishkuro
Copy link
Member

addressed in #2041

@yurishkuro yurishkuro closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:dependencies Update to dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants