Skip to content

Conversation

@howareyou
Copy link

And have extra , in cid

And have extra , in cid
Copy link
Owner

@dash0002 dash0002 left a comment

Choose a reason for hiding this comment

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

  • Can you update the CHANGELOG, README, etc
  • Can you double check with @eestrada? He was the original submitter of the code and it seems to have slipped through the cracks
  • Can you and @eestrada coordinate and submit a test case to cover this scenario?

skip = ns,
fail = nf,
error = ne,
cid = 'c%s' % (cid+1),
Copy link

Choose a reason for hiding this comment

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

@howareyou The trailing comma is syntactically correct in Python. It is useful for adding lines later without raising a syntax error when someone forgets that a comma was actually needed in the preceding.

Other than your personal preference, is there some reason for removing it? If not, then it complicates the git history without adding value. I suggest removing it from the commit.

Copy link

Choose a reason for hiding this comment

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

As far as the rest of the commit, it looks fine to me.

As to a test case, I am slammed with work and am working a lot of overtime. I am not sure I can carve out much time right now. I am happy to review whatever is submitted, but I can't make promises as to actually generating any code myself.

@howareyou
Copy link
Author

@dash0002 I am a newbie at Python:), and add trailing comma back

@dash0002
Copy link
Owner

dash0002 commented Jan 8, 2017

No worries @howareyou. Just glad to have people involved in the fork. I won't be merging until the test case is written, but there's no rush and it's a good experience also.

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