Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decaffeinate apm cli spec #64

Merged

Conversation

2colours
Copy link
Contributor

No description provided.

@2colours 2colours force-pushed the decaffeinate-apm-cli-spec branch from e9e71e8 to e7cc901 Compare March 28, 2023 14:11
@2colours 2colours marked this pull request as ready for review March 28, 2023 14:28
@2colours
Copy link
Contributor Author

The "successor" of #20 with a little twist: semicolons added and template strings preserved.

It's easier to run a diff in e.g VSCode, it maps the lines better.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light approval.

The code here looks solid. Some great decaf work opting for implicit functions rather than anonymous ones. Otherwise the other changes found don't seem of extreme significance, but mainly focusing on readability.

Other than waiting for tests to complete, this should be good to do.

@2colours
Copy link
Contributor Author

2colours commented Apr 3, 2023

Light approval.

The code here looks solid. Some great decaf work opting for implicit functions rather than anonymous ones. Otherwise the other changes found don't seem of extreme significance, but mainly focusing on readability.

Other than waiting for tests to complete, this should be good to do.

Thank you for the feedback.

Honestly, I don't know the API very well, this expect, for example. If you could give me some pointers what further changes you'd like to see, I can dive deeper into it.

@confused-Techie
Copy link
Member

@2colours
So for expect and it and things like beforeEach, beforeAll and that sort of thing you'll find in the spec files are all globals of jasmine.

Which is the testing framework listed here.

I'd love to link directly to some docs, but part of the hard thing is it looks like the Jasmine used in much of Atom's stuff was forked from a fork from a fork, ad nauseam. So means the docs aren't super clear.

But for understanding the basic principles, and the general syntax patterns we would expect feel free to look at their docs

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, finally a proper review. This looks good, some simplistic changes, but approving, and will go ahead and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants