feat: Add ratchet-from option #41

slarse · 2021-05-03T14:33:16Z

Fix #8

This PR finally adds the ratchet functionality. In essence, by adding e.g. ratchet-from: 'origin/main' to the with-block of the configuration for buildbreaker, only the lines changed from the main branch are considered.

codecov-commenter · 2021-05-03T14:46:17Z

Codecov Report

Merging #41 (1611db3) into main (8561a6e) will increase coverage by 0.42%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   92.71%   93.14%   +0.42%     
==========================================
  Files           6        6              
  Lines         151      175      +24     
  Branches       10       18       +8     
==========================================
+ Hits          140      163      +23     
- Misses         11       12       +1

Impacted Files	Coverage Δ
src/main.ts	`92.06% <96.00%> (+1.81%)`	⬆️
src/sorald.ts	`89.18% <100.00%> (+0.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8561a6e...1611db3. Read the comment docs.

This reverts commit 87c33bf.

slarse · 2021-05-04T07:29:24Z

__tests__/main.test.ts

+    '1854:Main.java:10:4:10:7',
+    '2164:Main.java:8:6:8:11'


Just for context, these are the unique identifiers for the two rule violations that are contained in the changed code.

algomaster99

Dropped a few comments. Rest looks good to me.

algomaster99 · 2021-05-04T18:29:28Z

src/main.ts


 import * as git from './git';
+import {ClosedRange} from './ranges';


I think line 10 will take care of this import.

This is a single-item import. With only line 10, I'd have to specify ranges.ClosedRange everywhere, as opposed to just ClosedRange. I don't know what's most common in TypeScript.

After looking around a bit more, there seems to be absolutely no consensus around how to do imports in TypeScript. It doesn't help that there are some 6 or so ways to do them. The only thing I can determine to be universally frowned upon is using require.

So, I'll go mostly with import * as whatever as that seems to be favored in other GHA implementations. I think we can use named imports as well when it makes sense.

Just as a side note on our discussion, I agree with avoiding * imports in Python, but that is very different from this * import. In python, import * from module imports all public top-level members of module (or everything defined in __all__) as individual objects, whereas import * as module ... in TS imports the module by some name (the closest to import module in Python that it seems to be possible to get, but really equivalent to import module as name).

It appears that combining star import with named import on a single line doesn't work. There appears to be a way to make it work with synthetic default imports, but restructuring the imports is starting to feel like a separate PR. Perhaps just defer this for later?

This is where a more structured language like Java is nice: there are standard imports, and there are static imports, and that's it :)

whereas import * as module ... in TS imports the module by some name

Yes, TS does not have a "default module name" like Python does.

Perhaps just defer this for later?

Restructuring would require a lot of thought and changes because there will be multiple export statements added and if you export something as default, you would need to decide which one. So you're right that there will be a lot of changed so you may defer this for later.

src/main.ts

algomaster99 · 2021-05-04T18:35:02Z

src/main.ts

@@ -62,11 +84,36 @@ export async function runSorald(
  return allRepairs;
 }

+function filterViolationSpecsByRatchet(


You forgot to add docstrings here. :)

As this function isn't exported, I figured it's like "private" in TypeScript, and therefore omitted the docstring.

According to meeting: add explanatory docstring.

slarse · 2021-05-06T10:55:19Z

@algomaster99 Ready for final review. You can see my comments on imports, I think we should just leave it for a separate PR and restructure all imports according to some format.

Right now, I'm thinking that types (e.g. ClosedRange) should be imported with named imports. That's because they typically appear in type annotations, and as you said, typing ranges.ClosedRange can make something like a function declaration much longer than it needs to be.

As for functions, I prefer using them with the qualified name (e.g. ranges.overlapsAny), as it makes it clear where they come from, and they aren't used in function headers or the like and so the added length isn't that much of a problem.

WDYT?

algomaster99 · 2021-05-06T11:26:30Z

I prefer using them with the qualified name (e.g. ranges.overlapsAny), as it makes it clear where they come from,

Keeping the conventions aside, this standalone makes sense and it indeed makes it clearer to identify where the function comes form.

can make something like a function declaration much longer than it needs to be.

But if you decide to create an instance first and then use the class methods, then this does not hold. Named export anyway sounds like a good idea. You could use default imports too but I think since it allows one to import it by any module name, the consistency might be lost if that module is used across a lot of file. For example, one file could import it as A, other B. Thus, I feel name imports will be better.

algomaster99 · 2021-05-06T11:30:28Z

__tests__/main.test.ts

-import * as process from 'process'
-import * as cp from 'child_process'
-import * as path from 'path'
+jest.setTimeout(20 * 1000);


A better alternative would be to set timeout for individual tests like given here.

@slarse , another person who thinks like us. :)

Good find, let's do that!

If there are many tests for which would need to do this fix, you can create a separate PR for it. Up to you.

Actually, since all of the main tests require a high timeout, it's probably just a better idea to set the timeout globally. DRY :)

In that case, you can configure this in jest.config.js. Haven't looked up if it is possible but if it is, I think you should consider it.

algomaster99 · 2021-05-06T11:36:12Z

src/main.ts

+    const repairedViolations: string[] = await runSorald(
+      source,
+      soraldJarUrl,
+      ratchetFrom ? ratchetFrom : undefined


Is this ternary operator required? Line 115 could be a string or undefined so accordingly the value can be set here. However, if the string is '' (empty string), you may want to set it to undefined.

Actually, line 115 is lying, it can't be undefined. It's always a string (empty string if it's not defined). So I need to change that confusing type annotation.

algomaster99 · 2021-05-06T11:37:27Z

@slarse dropped my comments. Once you address them, I will mark the PR as approved. :)

slarse · 2021-05-06T11:59:13Z

@algomaster99 comments addressed, merge if nothing major persists

slarse added 7 commits May 3, 2021 16:21

Implement ratchet backend

1ac6014

Add ratchet-from option to Action

0dffbaf

Document ratchet functionality in README

7f95f30

Fix type of ratchetFrom variable

bddc78a

Revert unnecessary change to download function

1d78fea

Remove rogue debug statement

4c70cd1

Update dist

3471295

slarse added 4 commits May 3, 2021 16:50

Fix crash on ratchet-from not being specified

87c33bf

Revert "Fix crash on ratchet-from not being specified"

f92f6a4

This reverts commit 87c33bf.

Replace falsy ratchetFrom with undefined

f36de9e

Update dist

9fdbc98

slarse requested a review from algomaster99 May 4, 2021 07:27

slarse commented May 4, 2021

View reviewed changes

algomaster99 reviewed May 4, 2021

View reviewed changes

slarse requested a review from algomaster99 May 5, 2021 08:35

slarse added 2 commits May 6, 2021 12:39

Add docstring to explain filterViolationSpecsByRatchet

9e60353

Update dist

2f0af8d

algomaster99 reviewed May 6, 2021

View reviewed changes

slarse added 2 commits May 6, 2021 13:42

Set global timeout only once in main.test.ts

938353c

Narrow type to string

1611db3

algomaster99 approved these changes May 6, 2021

View reviewed changes

algomaster99 merged commit f3ce916 into main May 6, 2021

algomaster99 deleted the issue/8-add-ratchet branch May 6, 2021 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ratchet-from option #41

feat: Add ratchet-from option #41

slarse commented May 3, 2021

codecov-commenter commented May 3, 2021 •

edited by codecov bot

Loading

slarse May 4, 2021

algomaster99 left a comment

algomaster99 May 4, 2021

slarse May 5, 2021

slarse May 6, 2021 •

edited

Loading

slarse May 6, 2021

algomaster99 May 6, 2021

algomaster99 May 4, 2021

slarse May 5, 2021

slarse May 6, 2021

slarse commented May 6, 2021

algomaster99 commented May 6, 2021

algomaster99 May 6, 2021 •

edited

Loading

slarse May 6, 2021

algomaster99 May 6, 2021

slarse May 6, 2021 •

edited

Loading

algomaster99 May 6, 2021

algomaster99 May 6, 2021

slarse May 6, 2021

algomaster99 commented May 6, 2021

slarse commented May 6, 2021 •

edited

Loading


		import * as git from './git';
		import {ClosedRange} from './ranges';

feat: Add ratchet-from option #41

feat: Add ratchet-from option #41

Conversation

slarse commented May 3, 2021

codecov-commenter commented May 3, 2021 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse commented May 6, 2021

algomaster99 commented May 6, 2021

algomaster99 May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 commented May 6, 2021

slarse commented May 6, 2021 • edited Loading

codecov-commenter commented May 3, 2021 •

edited by codecov bot

Loading

slarse May 6, 2021 •

edited

Loading

algomaster99 May 6, 2021 •

edited

Loading

slarse May 6, 2021 •

edited

Loading

slarse commented May 6, 2021 •

edited

Loading