PhoenicisOrg / scripts

Phoenicis scripts
GNU Lesser General Public License v3.0
64 stars 49 forks source link

Remove superfluous this.install() and this._wizard call #1117

Closed ImperatorS79 closed 4 years ago

ImperatorS79 commented 5 years ago

Description

If we get there the this.install has already been called

Ready for review

plata commented 5 years ago

I don't think that this will work. If the second install() is removed, you will not be able to change the Wine version for an existing prefix (iirc).

plata commented 5 years ago

The change is fine for me. I'm thinking if we shouldn't change

if (fileExists(workingContainerDirectory)) {
    const containerConfiguration = configFactory.open(workingContainerDirectory + "/phoenicis.cfg");

    distribution = containerConfiguration.readValue("wineDistribution", "upstream");
    architecture = containerConfiguration.readValue("wineArchitecture", "x86");

    const operatingSystem = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage();

    subCategory = distribution + "-" + operatingSystem + "-" + architecture;
    version = containerConfiguration.readValue("wineVersion");

    this.install(subCategory, version);
} else {
    print('Wine prefix "' + this.getWorkingContainer() + '" does not exist!');

    return "";
}

to

if (!fileExists(workingContainerDirectory)) {
    print('Wine prefix "' + this.getWorkingContainer() + '" does not exist!');

    return "";
}

const containerConfiguration = configFactory.open(workingContainerDirectory + "/phoenicis.cfg");

distribution = containerConfiguration.readValue("wineDistribution", "upstream");
architecture = containerConfiguration.readValue("wineArchitecture", "x86");

const operatingSystem = operatingSystemFetcher.fetchCurrentOperationSystem().getWinePackage();

subCategory = distribution + "-" + operatingSystem + "-" + architecture;
version = containerConfiguration.readValue("wineVersion");

this.install(subCategory, version);

Might be clearer. What do you think?

madoar commented 5 years ago
if (!fileExists(workingContainerDirectory)) {
    print('Wine prefix "' + this.getWorkingContainer() + '" does not exist!');

    return "";
}

Should we maybe throw an error/exception instead if this case occurs?

plata commented 5 years ago

Yes.

plata commented 5 years ago

There are issues in Codacy.

madoar commented 4 years ago

@ImperatorS79 this PR has a merge conflict.

plata commented 4 years ago

@madoar you have requested changes. Have they been addressed? If so, can we merge this?

madoar commented 4 years ago

There is still the new in throw new Error(...); missing.

plata commented 4 years ago

@ImperatorS79 can you fix that?

plata commented 4 years ago

@madoar please review again.