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

Fix tmTheme tool #5238

Merged
merged 5 commits into from
Jul 9, 2023
Merged

Fix tmTheme tool #5238

merged 5 commits into from
Jul 9, 2023

Conversation

NotLazy
Copy link
Contributor

@NotLazy NotLazy commented Jul 5, 2023

Issue #, if available: #5237

Description of changes: Update tmTheme tool to use non-deprecated plist.js methods

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@NotLazy
Copy link
Contributor Author

NotLazy commented Jul 5, 2023

sorry, this accidentally includes #5236

@akoreman akoreman linked an issue Jul 5, 2023 that may be closed by this pull request
@akoreman
Copy link
Contributor

akoreman commented Jul 5, 2023

hey, thanks for your contribution, could you decouple this PR from the other one so we can review/merge them separately.

Comparing the implementation of the new method and the old method this change looks good to me.

tool/tmtheme.js Outdated
callback(theme[0])
});
callback(parse(themeXml));
// parseString(themeXml, function(_, theme) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove these commented out old lines

@NotLazy
Copy link
Contributor Author

NotLazy commented Jul 9, 2023

hey, thanks for your contribution, could you decouple this PR from the other one so we can review/merge them separately.

Comparing the implementation of the new method and the old method this change looks good to me.

I would love to, but I'm not an expert with git. Is there a non-destructive way for me to do this?

I know I could rewrite the history of the repo, but I'm not sure of the effects on the PR and such.

@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (a6c8bf3) 87.22% compared to head (308b1ec) 87.24%.

❗ Current head 308b1ec differs from pull request most recent head ddf1199. Consider uploading reports for the commit ddf1199 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5238      +/-   ##
==========================================
+ Coverage   87.22%   87.24%   +0.01%     
==========================================
  Files         565      565              
  Lines       45253    45284      +31     
  Branches     6927     6928       +1     
==========================================
+ Hits        39471    39506      +35     
+ Misses       5782     5778       -4     
Flag Coverage Δ
unittests 87.24% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/undomanager.js 77.50% <ø> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@akoreman
Copy link
Contributor

akoreman commented Jul 9, 2023

In this case, easiest way is probably to just push a new commit to this branch on your fork where you delete the added lines to undo manager, then when we merge your PR, we will squash all your commits into a single commit on our master branch.

I went ahead and cleaned up the PR for you.

@akoreman akoreman merged commit 133345f into ajaxorg:master Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tool/tmTheme.js doesn't immediately work due to deprecation
2 participants