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.

Sign up for GitHub

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

Jump to bottom

Look for presets in base theme if current preset specified in current theme #389

Merged
merged 13 commits into from
Mar 18, 2020
Merged

Look for presets in base theme if current preset specified in current theme #389

merged 13 commits into from
Mar 18, 2020

Conversation

asvishnyakov
Copy link
Contributor

@asvishnyakov asvishnyakov commented Mar 16, 2020 •
edited
Loading

image

t13ka
t13ka previously approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@t13ka t13ka left a comment

Choose a reason for hiding this comment

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

acceptable

basilkot
basilkot previously approved these changes Mar 17, 2020
View reviewed changes
@asvishnyakov asvishnyakov force-pushed the fix/merge-settings-presets branch from f11731d to 4880f30 Compare March 18, 2020 05:04
@asvishnyakov asvishnyakov requested a review from t13ka March 18, 2020 05:14
vc-ci
vc-ci reviewed Mar 18, 2020
View reviewed changes
VirtoCommerce.LiquidThemeEngine/ShopifyLiquidThemeEngine.cs
@@ -358,20 +358,18 @@ public ValueTask<string> RenderTemplateAsync(string templateContent, string temp

JObject result;
var baseThemeSettings = new JObject();
var currentThemeSettings = result = GetCurrentSettingsPreset(InnerGetAllSettings(_themeBlobProvider, CurrentThemeSettingPath));
var currentThemeSettings = result = InnerGetAllSettings(_themeBlobProvider, CurrentThemeSettingPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

MAJOR Remove this useless assignment to local variable 'result'. rule

@vc-ci
Copy link
Contributor

vc-ci commented Mar 18, 2020

SonarQube analysis reported 6 issues

  • MAJOR 4 major
  • MINOR 1 minor
  • INFO 1 info

Watch the comments in this conversation to review them.

5 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR ShopifyLiquidThemeEngine.cs#L101: Extract this nested ternary operation into an independent statement. rule
  2. MAJOR ShopifyLiquidThemeEngine.cs#L153: Remove this commented out code. rule
  3. MAJOR ShopifyLiquidThemeEngine.cs#L264: Split this method into two, one handling parameters check and the other handling the asynchronous code. rule
  4. MINOR ShopifyLiquidThemeEngine.cs#L409: Remove this hardcoded path-delimiter. rule
  5. INFO ShopifyLiquidThemeEngine.cs#L310: Complete the task associated to this 'TODO' comment. rule

@tatarincev tatarincev merged commit 814ebb9 into dev Mar 18, 2020
@tatarincev tatarincev deleted the fix/merge-settings-presets branch March 18, 2020 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vc-ci vc-ci vc-ci left review comments

@tatarincev tatarincev tatarincev approved these changes

@basilkot basilkot basilkot left review comments

@t13ka t13ka Awaiting requested review from t13ka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@asvishnyakov @vc-ci @basilkot @tatarincev @t13ka