lint ruby drc code. #36

atorkmabrains · 2023-02-02T15:44:35Z

Fixes #14

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

Tests pass
Appropriate changes to README are included in PR

proppy · 2023-02-02T16:29:43Z

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

only keep in this PR: the changes that add lint to ci w/ dualgate modification to pass the checks
rebase the other PRs to include the corresponding lint modification for each deck check

proppy · 2023-02-02T16:29:56Z

That will make it easier to review things concurrently.

atorkmabrains · 2023-02-03T04:11:34Z

@FaragElsayed2 Could you please do that? My bad.

atorkmabrains · 2023-02-03T11:31:25Z

@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 · 2023-02-05T08:46:50Z

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

FaragElsayed2 · 2023-02-05T08:47:34Z

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

proppy · 2023-02-06T00:03:32Z

@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 · 2023-02-06T00:04:23Z

@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 · 2023-02-06T00:05:25Z

@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.

Makefile

.rubocop.yml

klayout/drc/rule_decks/tail.drc

klayout/drc/rule_decks/main.drc

proppy · 2023-02-06T00:17:37Z

klayout/drc/rule_decks/main.drc


 if $report
-    logger.info("GF180MCU Klayout DRC runset output at: %s" % [$report])
-    report("DRC Run Report at", $report)
+  logger.info(format('GF180MCU Klayout DRC runset output at: %s', $report))


string interpolation?

proppy · 2023-02-06T00:18:00Z

klayout/drc/rule_decks/main.drc

-else
-  BALL = "true"
-end # BALL
+BALL = if $ball == 'false'


do we want to default to be true here?

Can we keep the original structure of the code then and keep any config refactoring for #37?

Same as above

Are you intentionally keeping this a string rather that converting it to a boolean? I would assume that the rest of the rules would prefer to test this directly rather than doing string comparison?

Done, bool check function is used for string to boolean conversion.

proppy · 2023-02-06T00:19:13Z

klayout/drc/rule_decks/main.drc

-else
-  GOLD = "true"
-end # GOLD
+GOLD = if $gold == 'false'


do we want to default to be true here?

Can we keep the original structure of the code then and keep any config refactoring for #37?

Same as above

Ah I see, are you intentionally keeping this a string rather that converting it to a boolean? I would assume that the rest of the rules would prefer to test this directly rather than doing string comparison?

Done, bool check function is used for string to boolean conversion.

klayout/drc/rule_decks/main.drc

atorkmabrains · 2023-02-07T12:07:43Z

@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 · 2023-02-08T10:43:18Z

@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 · 2023-02-08T12:59:40Z

@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 · 2023-02-08T13:32:47Z

@FaragElsayed2 Could you please squish the commits history?

atorkmabrains · 2023-02-08T14:51:03Z

@proppy Commits are squashed to one commit as requested.

atorkmabrains · 2023-02-09T13:08:02Z

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

proppy · 2023-02-09T14:19:17Z

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

proppy

Just some nits left above better handing of the conditions per layers. But feel free to address them in a separate change.

Thanks for the hard work on this.

FaragElsayed2 · 2023-02-09T15:04:27Z

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

proppy · 2023-02-09T15:35:26Z

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

Thanks in advance

FaragElsayed2 · 2023-02-10T11:45:54Z

@proppy Done

atorkmabrains · 2023-02-10T12:01:08Z

Thanks @FaragElsayed2

atorkmabrains · 2023-02-10T12:02:50Z

@proppy Anything else?

klayout/drc/rule_decks/main.drc

atorkmabrains · 2023-02-10T16:03:54Z

@proppy Anything else?

atorkmabrains · 2023-02-10T16:04:28Z

@proppy Could you please merge?

proppy · 2023-02-10T16:26:41Z

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

Thanks for all the hard work on this!

atorkmabrains requested a review from proppy February 2, 2023 15:44

FaragElsayed2 force-pushed the lint_ruby branch from 16ed7ed to 531323a Compare February 5, 2023 08:06