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

chore(cosmic-swingset): repl.js imports getInterfaceOf/etc #2638

Merged
merged 2 commits into from
Mar 15, 2021

Conversation

warner
Copy link
Member

@warner warner commented Mar 14, 2021

.. rather than being given them on vatPowers, since those are going away.

refs #2637

@michaelfig I changed stringify to stop accepting a getInterfaceOf argument because eslint complained about the shadowed name. I didn't see any external importers of the file within cosmic-swingset/, so I removed the export. If there are any external importers of this file, they'll need to be updated for the new signature (I removed the argument from the middle, which is always troublesome). I figured a "cannot import" error would be more obvious than whatever would happen when one argument gets confused for another. As part of your review, please think about whether there are any other importers.

@warner warner added the cosmic-swingset package: cosmic-swingset label Mar 14, 2021
@warner warner requested a review from michaelfig March 14, 2021 22:41
@warner warner self-assigned this Mar 14, 2021
@warner
Copy link
Member Author

warner commented Mar 14, 2021

Ah, well apparently dapp-svelte-wallet uses it.

It looks like Debug.svelte only has a two-argument call to stringify(), so this change shouldn't affect it. I guess I'll re-enable the export and just hope that nothing else imports it. It's challenging to be confident about that.. the use of deep-imports makes me nervous.

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

I don't see the need to change the stringify API. Agreed that this could be cleaned up sometime, but I'm hesitant to make breaking changes without renaming the function.

@@ -8,7 +9,6 @@ import makeUIAgentMakers from './ui-agent';
export function stringify(
value,
spaces,
getInterfaceOf,
Copy link
Member

Choose a reason for hiding this comment

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

Why make an incompatible change to the API? Just rename the argument to avoid the shadowing, and default its value as in:

  gio = getInterfaceOf,

and then use gio within the stringify function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aye. I generally like to delete code when it's no longer needed, but I agree it's quicker and only slightly ugly to leave this argument in place.

.. rather than getting them from `vatPowers`, since those are going away.

refs #2637
@warner warner force-pushed the 2637-repl-getremoteinterface branch from 34b025c to 293e8bf Compare March 15, 2021 01:42
@warner warner requested a review from michaelfig March 15, 2021 01:42
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM!

@warner warner enabled auto-merge (rebase) March 15, 2021 05:27
@warner warner merged commit aaa0a39 into master Mar 15, 2021
@warner warner deleted the 2637-repl-getremoteinterface branch March 15, 2021 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmic-swingset package: cosmic-swingset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants