google / globalfoundries-pdk-libs-gf180mcu_fd_pv

Apache License 2.0
12 stars 6 forks source link

lint ruby drc code. #36

Closed atorkmabrains closed 1 year ago

atorkmabrains commented 1 year ago

Fixes #14

Please don't merge this PR before the other PRs.

proppy commented 1 year ago

can you invert the order of the commits in the history in order to:

proppy commented 1 year ago

That will make it easier to review things concurrently.

atorkmabrains commented 1 year ago

@FaragElsayed2 Could you please do that? My bad.

atorkmabrains commented 1 year ago

@proppy If you accept all the other open PRs and then this one in that order, everything will fall in place as you want. The rest of the tables will be correct moving forward and the new tables will be lint clean.

We won't be able to work on this until Sunday. If you can do the above that would be really helpful to speed up the process on our end.

FaragElsayed2 commented 1 year ago

@proppy I have removed all other files as requested and only kept dualgate drc file and linting checks for ruby files.

FaragElsayed2 commented 1 year ago

@proppy And @mithro Please add me as a collaborator for this project.

proppy commented 1 year ago

@proppy And @mithro Please add me as a collaborator for this project.

please ask mithro@ directly (let's keep the comment in this issue about the actual review)

proppy commented 1 year ago

@proppy I have removed all other files as requested and only kept dualgate drc file and linting checks for ruby files.

Thanks! that makes it a lot easier to review. Can you rebase 531323a on top of the main branch to clean up the history?

proppy commented 1 year ago

@proppy If you accept all the other open PRs and then this one in that order, everything will fall in place as you want. The rest of the tables will be correct moving forward and the new tables will be lint clean. We won't be able to work on this until Sunday. If you can do the above that would be really helpful to speed up the process on our end.

I prefer to review the PR as it is currently as it will allow us to review each further DRC PR idependently with the lint workflow enabled.

atorkmabrains commented 1 year ago

@proppy Could you please resolve any open discussions?

atorkmabrains commented 1 year ago

@proppy Could you please merge all the changes as @FaragElsayed2 has already either addressed all your questions or opened an issue with it?

proppy commented 1 year ago

@proppy Could you please merge all the changes as @FaragElsayed2 has already either addressed all your questions or opened an issue with it?

Resolved most of the comments (still a few left).

Can we make sure to squash the commits (since there is a lot of unrelated changes in the history)?

That should also make it easier to rebase the other PR on top of it.

atorkmabrains commented 1 year ago

@FaragElsayed2 Squashing commits on github to avoid too many commits for the PR: https://docs.github.com/en/desktop/contributing-and-collaborating-using-github-desktop/managing-commits/squashing-commits

atorkmabrains commented 1 year ago

@proppy I think @FaragElsayed2 has already addressed most of your concerns. Could you please merge the PR and open issues with any remaining issues as we agreed?

proppy commented 1 year ago

@atorkmabrains I agree that most of the comment have been addressed, but as suggested before I'd like the history to be squashed before merging (there is currently an history of 106 commits which are mostly unrelated to this change)

atorkmabrains commented 1 year ago

@FaragElsayed2 Could you please squish the commits history?

atorkmabrains commented 1 year ago

@proppy Commits are squashed to one commit as requested.

atorkmabrains commented 1 year ago

@proppy Could you please let me know if there is anything else?

proppy commented 1 year ago

It looks good, but please use a more descriptive commit message (no need to mention it's in one commit)

FaragElsayed2 commented 1 year ago

@proppy I think we have done with all issues, Could you please merge ?

proppy commented 1 year ago

Sure can you make sure to address it the last remaining comment and reword the commit message?

Thanks in advance

FaragElsayed2 commented 1 year ago

@proppy Done

atorkmabrains commented 1 year ago

Thanks @FaragElsayed2

atorkmabrains commented 1 year ago

@proppy Anything else?

atorkmabrains commented 1 year ago

@proppy Anything else?

atorkmabrains commented 1 year ago

@proppy Could you please merge?

proppy commented 1 year ago

@atorkmabrains yes, sorry I was just waiting for the CI to pass.

Thanks for all the hard work on this!