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

feat: add git scripts to Script Manager UI #1642

Merged
merged 12 commits into from Mar 21, 2023

Conversation

midgleyc
Copy link
Member

Add git scripts to the Script Manager GUI.

I don't use the GUI myself (just the CLI), so open for any comments on things that should work differently.

There are also, as yet, no git scripts in svnrepo.json, though I could change the github scripts that work over from SVN?

@midgleyc midgleyc added the git-scripts Install scripts using git label Mar 19, 2023
@midgleyc midgleyc requested a review from a team as a code owner March 19, 2023 16:34
@codecov
Copy link

codecov bot commented Mar 19, 2023

Codecov Report

Merging #1642 (2205229) into main (e6d2329) will decrease coverage by 0.05%.
The diff coverage is 2.42%.

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

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1642      +/-   ##
============================================
- Coverage     35.35%   35.30%   -0.05%     
+ Complexity    17874    17842      -32     
============================================
  Files          1064     1063       -1     
  Lines        164013   164014       +1     
  Branches      35060    35057       -3     
============================================
- Hits          57986    57908      -78     
- Misses        96411    96491      +80     
+ Partials       9616     9615       -1     
Impacted Files Coverage Δ
...c/net/sourceforge/kolmafia/persistence/Script.java 0.00% <0.00%> (ø)
...ourceforge/kolmafia/persistence/ScriptManager.java 0.00% <0.00%> (ø)
...t/sourceforge/kolmafia/scripts/svn/SVNManager.java 22.02% <0.00%> (-0.08%) ⬇️
...ourceforge/kolmafia/swingui/ScriptManageFrame.java 0.00% <0.00%> (ø)
.../kolmafia/swingui/widget/ShowDescriptionTable.java 0.00% <0.00%> (ø)
...orge/kolmafia/swingui/widget/TableCellFactory.java 0.00% <0.00%> (ø)
...t/sourceforge/kolmafia/scripts/git/GitManager.java 54.43% <14.28%> (-1.16%) ⬇️

... and 17 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd39bd0...1984408. Read the comment docs.

@Veracity0
Copy link
Contributor

I am experimenting. I didn't have any git scripts installed. So I found 8bit-relay.

I installed it it via git checkout loathers/8bit-relay.

> git list

loathers-8bit-relay

> git update loathers-8bit-relay

Updating project loathers-8bit-relay
Pull
No changes

In Script Manager, I can see loathers-8bit-relay in the Manage tab.
Right click for options:
Delete script -> Cannot find unique match for loathers-8bit-relay-main
Open forum thread -> nothing. Well, maybe if we had it in the repo, it would have a pointer to the forum thread.
Update this script -> Cannot find unique match for loathers-8bit-relay-main
Update all scripts -> Looked at all SVN scripts and also - successfully - at this one.

@midgleyc
Copy link
Member Author

Open forum thread -> nothing. Well, maybe if we had it in the repo, it would have a pointer to the forum thread.

That's what happens for the existing SVN scripts too.

Cannot find unique match for loathers-8bit-relay-main

I see what's happening here. main is the default branch, so it's optional, and it's not present in the installed version. We do know what the folder is, though, so we should be able to figure it out.

@jaadams5
Copy link
Contributor

Open forum thread -> nothing. Well, maybe if we had it in the repo, it would have a pointer to the forum thread.

That is expected behavior. At least it is what happens with SVN scripts that I have installed that are not referenced in svnrepo.json.

I would be concerned about changing the protocol on existing scripts until the local changes issue mentioned at https://kolmafia.us/threads/excavator-gausies-spading-script.25076/page-3#post-171587 is resolved.

Copy link
Contributor

@Veracity0 Veracity0 left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. I updated and tested the GUI again. Looks good!

I notice that GitManager catches - and logs - exceptions, returning false or null or whatever to indicate failure, whereas SVNManager just throws the exception, requiring the caller to catch and handle them.

I realize you wrote GitManager and it's just a cleaner implementation.
I think it is beyond the scope of this PR to sanitize SVNManager similarly.

Maybe someday.

@midgleyc midgleyc enabled auto-merge (squash) March 21, 2023 23:42
@midgleyc midgleyc merged commit 7a0ca40 into kolmafia:main Mar 21, 2023
3 checks passed
@midgleyc midgleyc deleted the script-manager-git branch March 22, 2023 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git-scripts Install scripts using git
Projects
None yet
3 participants