Refactor wrapper.ts to improve maintainability #614

stephen-slm · 2022-03-31T07:56:36Z

Why?

This change works on refactoring out the wrapper.ts file to allow improved maintainability, extensibility with support of easy typing. The current implementation is one continuous function call which allocates all the bindings with hard to follow scoping.

Any bindings of methods have been split out into the binding directly, currently containing the core (anything not compiler related) and compiler related bound functions.

Whats Next?

Since the wrapper is the main entry point for most consumers of the service, this should allow cleanly defined types to be exported with the package. Allowing language servers and IDEs to consume the types and expose them to the user for an improved experience.

The callback implementation to apply changes based on some additional input arguments can now be tackled as it can be harder to follow. I was going to do it in this PR but it was outside of the scope.

This has been done here #615

coveralls · 2022-03-31T08:02:38Z

Coverage increased (+0.8%) to 84.537% when pulling 864af41 on stephensli:refactor/wrapper into fe75220 on ethereum:master.

cameel

Unfortunately I did not manage to the through it all today so here's what I have so far.

Overall I did not find any real problems, mostly style/readability and places where naming could be clearer.

cameel · 2022-04-08T11:55:30Z

wrapper.ts

-    if (callbacks) {
-      assert(typeof callbacks === 'object', 'Invalid callback object specified.');
+  https.get(url, response => {
+    if (response.statusCode > 299 || response.statusCode < 200) {


This is actually a behavior change. I mean, it looks like a good change, but still, this PR is supposed to be just a refactor. Can you put any behavior changes in separate commits so that they stand out?

Ah the http codes, yeah I can convert it and add it again in another pr in the future.

I mean, a separate commit is enough. Does not have to be a full-blown PR. It's just stuff that's easy to miss in a bigger refactor :)

okay i'll add another commit at the end of this review to include it

Marking this unresolved since the commit is not there yet.

Ah yeah can do or another PR to keep it out, I will look to add this once we are fully happy with the other commits. I'm not a massive fan of this minimise commits flow when you can just squash commits during the merge which is why I left it out.

wrapper.ts

tsconfig.json

bindings/helpers.ts

bindings/compile.ts

wrapper.ts

cameel

I finally managed to get through it all.

I found two or three places that look like bugs. This indicates that we really don't have enough test coverage here.

Other than that, some comments about naming and overall structure. Some details need tweaking but I really like the final effect.

bindings/core.ts

bindings/compile.ts

wrapper.ts

bindings/index.ts

cameel · 2022-04-14T23:39:39Z

bindings/core.ts

+    alloc: bindAlloc(solJson),
+    license: bindLicense(solJson),
+    version: bindVersion(solJson),
+    reset: bindReset(solJson)


The current core module is a bag of everything. At the same time it does not include some stuff that I'd consider to be the core functionality of a compiler - i.e. compilation.

I don't think license and version really fit in core. I'd put them in a separate module (maybe compiler-info?)

Then core becomes more like utils or helpers.

Callbacks could really be in a module of their own. You already have them separated from compilation methods with headers.

Can do but callbacks are not bindings they are directly related to the compiling and should stay related (imo), will make this kind of change.

@cameel this creates a lot of additional implementation detail that adds more complexity than required. Taking those out results in passing another set of values deep into the callback code which is already complex.

ok. I thought moving them would be easy but if it does introduce that much complexity then let's leave them as is.

I'll take a closer look when I'm back next week but overall this is not a deal breaker.

So apart from callbacks there are still the first 2 points. core really does not sound right to me here since the stuff in it is so peripheral. Maybe it would make more sense as memory (containing alloc, reset, the string helpers) and info (containing license, version, semver, etc.). Or something like that.

The only way it makes sense to me to call these core is by saying they're all equal parts of libsolc.h, which is true but still...

But anyway, at this point I just want to get this merged so that's just an optional suggestion. If you can adjust it easily, great, if not, we'll go with what it's already there anyway.

I believe pulling it out was overly complicated since they would then depend on each other in some way. I will look again to see what can be improved, might be able to do something.

I believe this still stands.

matheusaaguiar

I have checked the old code against the new one to ensure that every function is present and it seems fine.

cameel

I left 3 comments but everything important has been addressed. I'm marking as approved since the review is done and we just need to finish discussing these small points and merge.

cameel · 2022-07-05T12:16:29Z

Also, please rebase on latest master. My test PRs went in recently.

r0qs · 2022-09-15T10:03:41Z

Hey, what is missing in this PR to be merged? Just the rebase on the latest master? It would be great to have this merged.

cameel · 2022-09-15T11:39:16Z

Basically rechecking the 3 comments I mentioned and rebasing. We're pretty much done here.

cameel · 2022-09-15T11:40:16Z

Then we need to rebase #615 on top of that and review.

stephen-slm · 2022-09-15T14:00:37Z

@r0qs and @cameel sorry guys, I have been super busy at work but its now on my plan to complete this on the weekend!

…enting

cameel

Thanks! I think we can merge it now and move on to the PR that adds types.

stephen-slm · 2022-09-20T18:15:41Z

Sounds good! I cannot merge it but ping me when its done and i'll rebase the next PR 🔥

cameel · 2022-09-20T20:11:30Z

Merged.

cameel mentioned this pull request Apr 2, 2022

Add proper unit tests to translate.js #414

Open

stephen-slm mentioned this pull request Apr 3, 2022

Provide type support to wrapper.ts #615

Closed

stephen-slm changed the title ~~WIP: Refactor wrapper.ts to improve maintainability~~ Refactor wrapper.ts to improve maintainability Apr 3, 2022

stephen-slm marked this pull request as ready for review April 4, 2022 11:44

cameel reviewed Apr 8, 2022

View reviewed changes

stephen-slm requested a review from cameel April 13, 2022 07:55

cameel reviewed Apr 14, 2022

View reviewed changes

stephen-slm requested a review from cameel May 7, 2022 10:28

stephen-slm mentioned this pull request May 17, 2022

Add the missing common/helpers.ts to package.json #623

Merged

matheusaaguiar approved these changes May 20, 2022

View reviewed changes

cameel approved these changes Jul 5, 2022

View reviewed changes

Refactor wrapper.ts to improve maintainability by splitting and docum…

864af41

…enting

cameel approved these changes Sep 20, 2022

View reviewed changes

cameel merged commit 05a1955 into ethereum:master Sep 20, 2022

cameel added the refactor label Sep 20, 2022

stephen-slm deleted the refactor/wrapper branch September 22, 2022 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor wrapper.ts to improve maintainability #614

Refactor wrapper.ts to improve maintainability #614

stephen-slm commented Mar 31, 2022 •

edited

Loading

coveralls commented Mar 31, 2022 •

edited

Loading

cameel left a comment •

edited

Loading

cameel Apr 8, 2022

stephen-slm Apr 12, 2022

cameel Apr 14, 2022

stephen-slm Apr 19, 2022

cameel Jul 5, 2022

stephen-slm Jul 5, 2022

cameel left a comment •

edited

Loading

cameel Apr 14, 2022 •

edited

Loading

stephen-slm Apr 25, 2022

stephen-slm May 1, 2022

cameel May 2, 2022

cameel Jul 5, 2022

stephen-slm Jul 5, 2022

stephen-slm Sep 18, 2022

matheusaaguiar left a comment

cameel left a comment •

edited

Loading

cameel commented Jul 5, 2022

r0qs commented Sep 15, 2022

cameel commented Sep 15, 2022

cameel commented Sep 15, 2022 •

edited

Loading

stephen-slm commented Sep 15, 2022

cameel left a comment

stephen-slm commented Sep 20, 2022

cameel commented Sep 20, 2022

Refactor wrapper.ts to improve maintainability #614

Refactor wrapper.ts to improve maintainability #614

Conversation

stephen-slm commented Mar 31, 2022 • edited Loading

Why?

Whats Next?

coveralls commented Mar 31, 2022 • edited Loading

cameel left a comment • 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

Choose a reason for hiding this comment

cameel left a comment • edited Loading

Choose a reason for hiding this comment

cameel Apr 14, 2022 • 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

Choose a reason for hiding this comment

matheusaaguiar left a comment

Choose a reason for hiding this comment

cameel left a comment • edited Loading

Choose a reason for hiding this comment

cameel commented Jul 5, 2022

r0qs commented Sep 15, 2022

cameel commented Sep 15, 2022

cameel commented Sep 15, 2022 • edited Loading

stephen-slm commented Sep 15, 2022

cameel left a comment

Choose a reason for hiding this comment

stephen-slm commented Sep 20, 2022

cameel commented Sep 20, 2022

stephen-slm commented Mar 31, 2022 •

edited

Loading

coveralls commented Mar 31, 2022 •

edited

Loading

cameel left a comment •

edited

Loading

cameel left a comment •

edited

Loading

cameel Apr 14, 2022 •

edited

Loading

cameel left a comment •

edited

Loading

cameel commented Sep 15, 2022 •

edited

Loading