Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Use specified solc version range to determine which solc to download #1480

Merged
merged 4 commits into from
Dec 4, 2018

Conversation

eggplantzzz
Copy link
Contributor

Update the compilerSupplier to support version constraints.

@gnidan gnidan changed the title Byoc version issue Use specified solc version range to determine which solc to download Nov 29, 2018
@coveralls
Copy link

coveralls commented Nov 29, 2018

Coverage Status

Coverage decreased (-0.06%) to 67.863% when pulling 9a6cc0f on byoc-version-issue into bfb03e8 on next.

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple thoughts

if (satisfyingVersions.length > 0) {
return satisfyingVersions.reduce((newestVersion, version) => {
if (semver.gtr(version, newestVersion)) return version;
}, "0.0.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

nice reduce

solcVersion => {
if (semver.satisfies(solcVersion, version)) return solcVersion;
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use const instead of let here if you just tack the filter onto the end... can even get rid of the extra if in the filter

Suggested change
);
).filter(v => v);

Copy link
Contributor Author

@eggplantzzz eggplantzzz Nov 30, 2018

Choose a reason for hiding this comment

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

Oh that is a nice, concise solution.

if (semver.satisfies(solcVersion, version)) return solcVersion;
}
);
satisfyingVersions = satisfyingVersions.filter(solcVersion => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
satisfyingVersions = satisfyingVersions.filter(solcVersion => {

}
);
satisfyingVersions = satisfyingVersions.filter(solcVersion => {
if (solcVersion) return solcVersion;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (solcVersion) return solcVersion;

);
satisfyingVersions = satisfyingVersions.filter(solcVersion => {
if (solcVersion) return solcVersion;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
});

Copy link
Contributor

Choose a reason for hiding this comment

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

(I wish I could do a multi-line suggestion)

.catch(err => {
spinner.stop();
throw self.errors("noRequest", url, err);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Wanna make this await-y?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@eggplantzzz eggplantzzz merged commit 83e46fe into next Dec 4, 2018
@eggplantzzz eggplantzzz deleted the byoc-version-issue branch December 4, 2018 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants