Skip to content

Conversation

@umesh-timalsina
Copy link
Contributor

No description provided.

@umesh-timalsina umesh-timalsina requested a review from brollb July 14, 2020 16:31
1. Minor refactor of ordering of functions
Copy link
Contributor

@brollb brollb left a comment

Choose a reason for hiding this comment

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

A couple minor comments but nothing big.

There is one other minor thing that would probably be good but can be part of their own PR as it isn't pressing. I don't think we need to record this._metaNodesMap. The methods that use it could be rewritten to just use the client. For example, getMetaType could be rewritten to get the ID and then use getNode from the client. Checking if the node inherits from "Metadata" could also be checked by using a convenience method that walks up the inheritance tree and checks for a node that is both part of the meta and has the given name. This would likely be more performant than the current approach. Anyway, it isn't a huge deal so it doesn't need to be part of this PR.

1. Remove _metaNodesMap from FigureExtractor
2. Use .find instead of .filter followed by .pop in getMetadataChildrenIds
@umesh-timalsina
Copy link
Contributor Author

For example, getMetaType could be rewritten to get the ID and then use getNode from the client. Checking if the node inherits from "Metadata" could also be checked by using a convenience method that walks up the inheritance tree and checks for a node that is both part of the meta and has the given name. This would likely be more performant than the current approach.

This has been addressed.

@brollb brollb merged commit d383d00 into master Jul 14, 2020
@brollb brollb deleted the 1764-fig-extractor-metadata branch July 14, 2020 18:47
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