tensorflow-recorder icon indicating copy to clipboard operation
tensorflow-recorder copied to clipboard

Published 20 hours ago • verified publisher icon google

Added lowercase split value and passing tests#42

Open Samad999777 opened this issue 4 years ago • 16 comments
trafficstars

This pull request takes care of the lower case support issue and enables the users to follow the normal convention as mentioned in #42

This is mainly a cosmetic change and the dataset labels such as 'TRAIN' ,'TEST', 'VALIDATE' are not compatible as lower cases versions. It does not affect the broader scope of the project. however it may require documentation changes.

Fixes #42

Cosmetic change. as explained in summary above.

Please delete options that are not relevant.

  • [ Y ] New feature (non-breaking change which adds functionality)
  • [ Y ] This change requires a documentation update

Checklist

  • [Y] My code adheres to the Google Python Style Guide
  • [Y] I ran make test and all tests pass
  • [Y] I ran the tool and verified the change works
  • [Y] I have adequately commented my code, particularly in hard-to-understand areas
  • [Y] My changes generate no new warnings
  • [Y] I have corrected any misspellings in my code
  • [Y] (For feature) I made sure that I'm merging to the dev branch

Samad999777 avatar Jun 29 '21 16:06 Samad999777

@cfezequiel kindly review the new commits

Samad999777 avatar Jul 10 '21 15:07 Samad999777

@cfezequiel Kindly review the changes

Samad999777 avatar Aug 08 '21 13:08 Samad999777

@cfezequiel Did you review the changes ?

Samad999777 avatar Aug 22 '21 12:08 Samad999777

@cfezequiel alright, I will make the changes and hopefully we can publish these last changes as well.

Samad999777 avatar Jan 14 '22 19:01 Samad999777

@cfezequiel can you kindly review again and approve ?

Samad999777 avatar Jan 14 '22 19:01 Samad999777

@Samad999777 looks like there is a conflict with one of the files. Could you please check?

cfezequiel avatar Jan 14 '22 19:01 cfezequiel

@cfezequiel sure, i'll review

Samad999777 avatar Jan 14 '22 19:01 Samad999777

@cfezequiel I've resolved the conflict. Can you review ?

Samad999777 avatar Jan 14 '22 19:01 Samad999777

@Samad999777 got it. There seems to be a cla issue but it looks like something I need to resolve on my end.

cfezequiel avatar Jan 14 '22 21:01 cfezequiel

@cfezequiel yes, i think you need to re-submit your CLA.

Samad999777 avatar Jan 14 '22 21:01 Samad999777

@cfezequiel can you kindly take care of the CLA so these changes can be pushed?

Samad999777 avatar Jan 16 '22 20:01 Samad999777

@Samad999777 I may need to update the commit in your PR and push the change to your branch to address the CLA issue (basically remove the unregistered email address there). Are you ok with that?

cfezequiel avatar Jan 20 '22 18:01 cfezequiel

@cfezequiel As long as i get to be the contributer of this pull request in the project. I am very much okay with it.

Samad999777 avatar Jan 20 '22 18:01 Samad999777

@Samad999777 are you able to revert this commit: "Merge branch 'dev' into main" and do a rebase against the dev branch instead? That way, your commits come out on top in the commit history.

cfezequiel avatar Jan 20 '22 20:01 cfezequiel

@cfezequiel I have reverted the commit: "Merge branch 'dev' into main". However when i attempted to rebase it says that it is already upto date [see image below]. Am i doing this right ? image

Samad999777 avatar Jan 25 '22 19:01 Samad999777

@Samad999777 Thanks for the change. Have you tried rebasing the main branch in your fork with this repo's dev branch? PRs should be made off of the dev branch unless it's a hotfix.

cfezequiel avatar Jan 26 '22 19:01 cfezequiel