Skip to content

Add danger for showing dts-critic suggestions#44430

Merged
sandersn merged 37 commits into
masterfrom
add-danger-for-suggestions
May 11, 2020
Merged

Add danger for showing dts-critic suggestions#44430
sandersn merged 37 commits into
masterfrom
add-danger-for-suggestions

Conversation

@sandersn

@sandersn sandersn commented May 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

I have no idea if this will work; this is based on an idea by @orta but I'm not using Github Actions or yarn.
(original PR is here: typescript-cheatsheets/react#225)

@sandersn

sandersn commented May 1, 2020

Copy link
Copy Markdown
Contributor Author

Wow, good job danger! At least you ran!

(going to write a real dangerfile now)

@sandersn sandersn changed the title First attempt Add danger for showing dts-critic suggestions May 1, 2020
@orta orta force-pushed the add-danger-for-suggestions branch from e8cd9ef to b613c87 Compare May 2, 2020 12:36
@sandersn

sandersn commented May 4, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the dangerfile.ts @orta. Lesson learned: it's hard to add a gitignored .js file to DT, even on purpose.

@orta

orta commented May 4, 2020

Copy link
Copy Markdown
Collaborator

fixes DefinitelyTyped/dt-mergebot#27

@weswigham weswigham marked this pull request as draft May 4, 2020 23:13
@orta

orta commented May 5, 2020

Copy link
Copy Markdown
Collaborator

Danger should say nothing when there are no suggestions (then the GitHub comment will be deleted)

@typescript-bot

typescript-bot commented May 5, 2020

Copy link
Copy Markdown
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊
master #44430 diff
Batch compilation
Memory usage (MiB) 92.5 94.5 +2.1%
Type count 15150 15150 0%
Assignability cache size 35783 35783 0%
Language service
Samples taken 1329 1329 0%
Identifiers in tests 1329 1329 0%
getCompletionsAtPosition
    Mean duration (ms) 318.1 314.6 -1.1%
    Mean CV 9.4% 9.3%
    Worst duration (ms) 449.0 432.7 -3.6%
    Worst identifier default removeEventListener
getQuickInfoAtPosition
    Mean duration (ms) 314.5 311.1 -1.1%
    Mean CV 9.6% 9.9% +2.3%
    Worst duration (ms) 425.6 410.0 -3.6%
    Worst identifier el temp

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label May 5, 2020
@sandersn

sandersn commented May 5, 2020

Copy link
Copy Markdown
Contributor Author

@orta I'll eventually remove the "no suggestions found" message, but I have to explicitly handle the case when dts-critic doesn't produce suggestions, and I wanted to make sure my predicate was working.

@sandersn

sandersn commented May 5, 2020

Copy link
Copy Markdown
Contributor Author

The suggestions are only showing up on Travis for some reason. I'm going to move danger over there for now and back to pipelines when I figure out why it doesn't have suggestions.

@orta

orta commented May 8, 2020

Copy link
Copy Markdown
Collaborator

I've given it a bit of a tweak

@gabritto

gabritto commented May 8, 2020

Copy link
Copy Markdown
Contributor

@sandersn very nice to see this being implemented :)

@orta

orta commented May 8, 2020

Copy link
Copy Markdown
Collaborator

Might need to improve dts critic to have richer line/file references, the unpkg link for aframe is dist/index.js but the file reference I was working with is index.d.ts and converting that to a JS file isn't enough

@sandersn
Better unpkg/file linking
0b4a23c 
1. only show files if more than one was edited.
2. put top-level unpkg on top-level package heading
@sandersn

sandersn commented May 11, 2020

Copy link
Copy Markdown
Contributor Author

All right, this is where I want it for now. Since I'm on duty this week, I'll take some time to figure out why dts-critic isn't logging suggestions on Azure so I can switch danger off of Travis.

For now I want to merge this so I can see how well it works.

@sandersn sandersn merged commit 5f0ca35 into master May 11, 2020
@sandersn sandersn deleted the add-danger-for-suggestions branch May 11, 2020 18:07
andreialecu pushed a commit to andreialecu/DefinitelyTyped that referenced this pull request May 12, 2020
@sandersn @orta @andreialecu
Add danger for showing dts-critic suggestions (DefinitelyTyped#44430)
d64bf1e 
* First attempt

* cwd churn

* cwd churn 2

* cwd churn 3

* cwd churn 4

* cwd churn 5

* cwd churn 6

* dump ls

* Add a TS dangerfile

* Faff with the CI

* try to read suggestions

* add bogus aframe change

* use correct dir

* handle no suggestions

* delete more of aframe to get dts-critic to notice

* now try yauzl

* move danger to travis for now

* Revert "delete more of aframe to get dts-critic to notice"

This reverts commit 65ea09f.

* Revert "add bogus aframe change"

This reverts commit a6546bd.

* first try at formatting with markdown

* maybe it's a line-oriented format

* rebreak aframe

* Better formatting

Still not best

* better wording, maybe better formatting

* maybe I need a space to get autonumbering

* even better formatting

* special-case for >5 properties

* Use a summary discolosure

* Adds links to unpkg and the dts

* Fix the unpkg link

* Better unpkg/file linking

1. only show files if more than one was edited.
2. put top-level unpkg on top-level package heading

* fix formatting and single-file skip

* url needs trailing /

* Revert "now try yauzl"

This reverts commit 08ab238.

* Revert "rebreak aframe"

This reverts commit 6ceed76.

Co-authored-by: Orta Therox <[email protected]>
@peterblazejewicz peterblazejewicz mentioned this pull request May 23, 2020
Closed
13 tasks
jjballano-qatium pushed a commit to jjballano-qatium/DefinitelyTyped that referenced this pull request Jun 16, 2020
@sandersn @orta @jjballano-qatium
Add danger for showing dts-critic suggestions (DefinitelyTyped#44430)
ada489f 
* First attempt

* cwd churn

* cwd churn 2

* cwd churn 3

* cwd churn 4

* cwd churn 5

* cwd churn 6

* dump ls

* Add a TS dangerfile

* Faff with the CI

* try to read suggestions

* add bogus aframe change

* use correct dir

* handle no suggestions

* delete more of aframe to get dts-critic to notice

* now try yauzl

* move danger to travis for now

* Revert "delete more of aframe to get dts-critic to notice"

This reverts commit 65ea09f.

* Revert "add bogus aframe change"

This reverts commit a6546bd.

* first try at formatting with markdown

* maybe it's a line-oriented format

* rebreak aframe

* Better formatting

Still not best

* better wording, maybe better formatting

* maybe I need a space to get autonumbering

* even better formatting

* special-case for >5 properties

* Use a summary discolosure

* Adds links to unpkg and the dts

* Fix the unpkg link

* Better unpkg/file linking

1. only show files if more than one was edited.
2. put top-level unpkg on top-level package heading

* fix formatting and single-file skip

* url needs trailing /

* Revert "now try yauzl"

This reverts commit 08ab238.

* Revert "rebreak aframe"

This reverts commit 6ceed76.

Co-authored-by: Orta Therox <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sandersn @orta @typescript-bot @gabritto