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

Change the "mirror" command to completely mirror gCLI #942

Closed
wants to merge 27 commits into from

Conversation

libraryaddict
Copy link
Contributor

Marked as draft, pending resolution of conversation.

This change replaces the previous usage of mirror which is not well understood, and doesn't seem of interest to most users.

This change allows you to log all gCLI output to a file, allowing you to open it in a browser and view the full html of your session.

@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

Merging #942 (8042682) into main (338a45b) will increase coverage by 0.04%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #942      +/-   ##
============================================
+ Coverage     27.21%   27.25%   +0.04%     
- Complexity    12795    12819      +24     
============================================
  Files          1029     1029              
  Lines        160441   160443       +2     
  Branches      35313    35314       +1     
============================================
+ Hits          43656    43725      +69     
+ Misses       108799   108723      -76     
- Partials       7986     7995       +9     
Impacted Files Coverage Δ
...orge/kolmafia/textui/command/MirrorLogCommand.java 64.70% <40.00%> (+45.95%) ⬆️
src/net/sourceforge/kolmafia/RequestLogger.java 19.60% <60.00%> (+1.76%) ⬆️
...et/sourceforge/kolmafia/objectpool/Concoction.java 59.48% <0.00%> (-0.19%) ⬇️
...sourceforge/kolmafia/textui/langserver/Script.java 85.10% <0.00%> (+3.19%) ⬆️
...net/sourceforge/kolmafia/utilities/NullStream.java 26.66% <0.00%> (+3.33%) ⬆️
.../net/sourceforge/kolmafia/utilities/LogStream.java 20.00% <0.00%> (+4.44%) ⬆️
...ourceforge/kolmafia/persistence/SkillDatabase.java 35.62% <0.00%> (+5.69%) ⬆️

Continue to review full report at Codecov.

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

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 2, 2022

I still don't have a failing test.

I believe the hypothesis that the presence of absence of HTML influences the logging is incorrect. I mirror'd prefref and got raw html. I renamed the file to be .html and it rendered as expected.

I'm willing to force the extension to be txt unless the USER explicitly typed .htm or .html

@libraryaddict
Copy link
Contributor Author

I still don't have a failing test.

I believe the hypothesis that the presence of absence of HTML influences the logging is incorrect. I mirror'd prefref and got raw html. I renamed the file to be .html and it rendered as expected.

I'm willing to force the extension to be txt unless the USER explicitly typed .htm or .html

The test will fail if you revert the changes in request logger

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 2, 2022

OK your test is failing for me but the problem is the test. getMirrorLog is not actually reading the file. More later since I am waiting for a meeting that just started.

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 2, 2022

Bolloxed up my changes to RequestLogger...

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 2, 2022

I think my change to RequestLogger is equivalent to yours but it took me a while to see that. I didn't change printList similarly because I wasn't certain things weren't being written twice. The test should be restored so it allows htm and html if the user specified the ext. but I haven't done that yet. I'd like some real world testing but I think we are closer than we were before.

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 3, 2022

I'd like a test for printList and then I think I'd approve this, or more likely, open it for review first :-)

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 4, 2022

I'm not happy.

mirroring ashq print("Mirror test."); now adds
in the mirror which AFAIK is wrong.

@libraryaddict
Copy link
Contributor Author

I'm not happy.

mirroring ashq print("Mirror test."); now adds in the mirror which AFAIK is wrong.

Actually, that's unchanged behavior.

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 5, 2022

I'm not happy.
mirroring ashq print("Mirror test."); now adds in the mirror which AFAIK is wrong.

Actually, that's unchanged behavior.

Actually GitHub ate my "less than b r greater than".

I was trying to say

mirroring ashq print("Mirror test."); now adds an HTML break in the mirror.

There's probably a test here.

@libraryaddict
Copy link
Contributor Author

I'm not happy.
mirroring ashq print("Mirror test."); now adds in the mirror which AFAIK is wrong.

Actually, that's unchanged behavior.

Actually GitHub ate my "less than b r greater than".

I was trying to say

mirroring ashq print("Mirror test."); now adds an HTML break in the mirror.

There's probably a test here.

Yes, which is what the html test is? That test would previously fail.

@jaadams5
Copy link
Contributor

jaadams5 commented Aug 5, 2022

I wonder if we are at cross-purposes?

My goals:

Everything that is written to the gCLI when mirror is enabled is also written to the mirror file.

Nothing is duplicated in the mirror unless it was duplicated in the gCLI.

Nothing is in the mirror unless it was also in the gCLI.

gCLI text with embedded HTML in the gCLI will be written as text with HTML tags to the mirror.

gCLI text with no embedded HTML will not have embedded HTML in the mirror.

In addition to automated testing a human using a text editor and with the gCLI on screen should be able to confirm that the gCLI and the mirror have the same content.

@libraryaddict
Copy link
Contributor Author

I wonder if we are at cross-purposes?

My goals:

Everything that is written to the gCLI when mirror is enabled is also written to the mirror file.

Nothing is duplicated in the mirror unless it was duplicated in the gCLI.

Nothing is in the mirror unless it was also in the gCLI.

gCLI text with embedded HTML in the gCLI will be written as text with HTML tags to the mirror.

gCLI text with no embedded HTML will not have embedded HTML in the mirror.

In addition to automated testing a human using a text editor and with the gCLI on screen should be able to confirm that the gCLI and the mirror have the same content.

That sounds right.

I understand that your problem is the line break? That's a part of what's written to gcli which confuses me on what you're expecting

@jaadams5 jaadams5 mentioned this pull request Aug 5, 2022
@jaadams5
Copy link
Contributor

jaadams5 commented Aug 5, 2022

I made a version of MirrorTest that executes against main, without these changes.

#951

At this point I'm not certain our tests for this change are correct but step 1 is to understand the existing behaviour.

Tweaked some of the test assertions to have more useful error messages if they fail.  But left some others, at least until someone objects.
Another working test.
Some cleanup regarding expectation of HTML
@jaadams5
Copy link
Contributor

Closing in favor of #981

@jaadams5 jaadams5 closed this Aug 13, 2022
@libraryaddict libraryaddict deleted the mirror-gcli branch August 26, 2022 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants