Skip to content

Conversation

stollr
Copy link

@stollr stollr commented Nov 2, 2017

The improved sentence in the code comment misses a word, making the comment less reasonable.

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

I think the current text is correct, although it could be improved.

The argument of findDefinition() can be an ID or alias ... so you get the definition with the "app.user_config_manager" ID or alias.

How could we reword this to make it easier to understand?

@stollr
Copy link
Author

stollr commented Nov 3, 2017

The argument of findDefinition() can be an ID or alias

Yes, that's why I changed the sentence to "get the definition [...] by ID or alias"

I have just taken the sentence from the doc-comment of the findDefinition() method. See here

@javiereguiluz
Copy link
Member

I see ... but I think the new sentence is missing something. Now it reads: ... with the XXXX by ID or alias. There's something missing after the XXX or the the must be removed. Maybe it's more clear like this:

// get the definition whose ID or alias is "app.user_config_manager" 
@stollr
Copy link
Author

stollr commented Nov 6, 2017

Oh dear, after reading the sentence once again I realize that I have got it totally wrong. Sorry for the noise, I think it is okay, like it is. I'll close this PR.

@stollr stollr closed this Nov 6, 2017
@stollr stollr deleted the fixed_definitions_doc branch November 6, 2017 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants