Skip to content

Conversation

@kroenlein
Copy link
Collaborator

Added a function to make the index used in the substitute_objects method.

Follow-up to #131.

@kroenlein kroenlein requested a review from lkubie March 29, 2021 15:40
Copy link
Contributor

@lkubie lkubie left a comment

Choose a reason for hiding this comment

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

Please use a more descriptive variable name for make_index(). The variable name Obj does not make clear that this can accept one of several data types.

In the docstring, please be more expressive as to what this function can accept. This will allow users to use the method with more confidence

@kroenlein
Copy link
Collaborator Author

Please use a more descriptive variable name for make_index(). The variable name Obj does not make clear that this can accept one of several data types.

In the docstring, please be more expressive as to what this function can accept. This will allow users to use the method with more confidence

The docstring is now more expressive about the potential argument types.

The variable name for make_index is still obj since that's consistent with the other methods in the file. A more correctly scoped word for a wide range of types than the generic "object" is not clear, but suggestions would be welcome.

@kroenlein kroenlein requested a review from lkubie March 30, 2021 20:00
@kroenlein kroenlein merged commit 55de9f7 into master Apr 2, 2021
@kroenlein kroenlein deleted the feature/build-index branch April 2, 2021 16:52
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.

4 participants