search

hms-networks / flexy-canary-connector

Apache License 2.0
0 stars 0 forks source link

Dev/1.0.0 #19

Closed oliver-walker-hms closed 1 year ago

oliver-walker-hms commented 1 year ago

There are multiple TODOs in here that will be addressed in future PRs

TomKimsey commented 1 year ago

It seems like a lot of this functionality has Todos that seem to indicate that the code will change. Im not quite sure why its being done this way but it would be nicer as a reviewer and for the git history to see smaller chunks of finished code committed rather than larger files of more templated out code. As a reviewer im unsure what is "final" and what is just a placeholder.

alexjhawk commented 1 year ago

It seems like a lot of this functionality has Todos that seem to indicate that the code will change. Im not quite sure why its being done this way but it would be nicer as a reviewer and for the git history to see smaller chunks of finished code committed rather than larger files of more templated out code. As a reviewer im unsure what is "final" and what is just a placeholder.

This is being done to improve the collaboration between developers on this project. It quickly becomes extremely difficult to manage shared code that is rapidly evolving without source control.

Alternatively, do you think merging this to a 'temporary' main branch makes sense, which would allow us to clean up the overall Git history at the v1.0.0 release time?

oliver-walker-hms commented 1 year ago

It seems like a lot of this functionality has Todos that seem to indicate that the code will change. Im not quite sure why its being done this way but it would be nicer as a reviewer and for the git history to see smaller chunks of finished code committed rather than larger files of more templated out code. As a reviewer im unsure what is "final" and what is just a placeholder.

The comments should be a lot clearer on what is a TODO and what is a comment. If there is no "TODO:" it is a normal comment.

oliver-walker-hms commented 1 year ago

With the amount of comments on this PR it might help to start closing resolved comments after confirming they are resolved.

TomKimsey commented 1 year ago

With the amount of comments on this PR it might help to start closing resolved comments after confirming they are resolved.

I have been when rereviewing.

alexjhawk commented 1 year ago

@it-hms I merged with your previous approval, as the following changes were minimal/minor. Let me know if there is anything you'd like me to reevaluate and edit.

@oliver-walker-hms Per our conversation yesterday, I addressed additional review comments/suggestions and merged the PR after approval. We must also address Javadoc errors within the project to resolve build action errors resulting from Javadoc build failure(s). Some may be upstream in the extensions library, so I'm not sure why this is only coming up now.