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 #2400 -- Removed ancient wisdom #2401

Merged
merged 3 commits into from
Jan 28, 2019

Conversation

mathuin
Copy link
Contributor

@mathuin mathuin commented Jan 8, 2019

Throughout the documentation, there are many references to very old
versions of kOS (and other addons) which are no longer relevant. There
are also references to deprecated functionality and tutorials on
performing tasks "by hand" which are now built into the software. This
content has been removed.

Throughout the documentation, there are many references to very old 
versions of kOS (and other addons) which are no longer relevant.  There 
are also references to deprecated functionality and tutorials on 
performing tasks "by hand" which are now built into the software.  This 
content has been removed.
@Dunbaratu
Copy link
Member

I disagree with some of these edits.
The removals make sense if you're thinking the purpose of the docs is only "teach someone new to the mod how to write a script from scratch" and that's it.
But some of the removals do not make sense if you're thinking the docs will be used by someone who comes across an existing script on the web and is trying to figure out why on earth it doesn't work when they try it. (And the reason it doesn't work is that its using something that got deprecated.)

The main reason for some of these old comments is precisely that scenario. They tell you why an existing old script doesn't work anymore, and what about it needs to be changed to get it working.

@mathuin
Copy link
Contributor Author

mathuin commented Jan 8, 2019

You make an excellent point with respect to the edits that removed deprecated features. I'm not sure how many folks come across scripts from old YouTube videos or the like and try to run them years later, but it's probably non-zero. Do you think it's better to keep those in place, or move them to another part of the docs? Also, I would really appreciate it if you could review the PR and mark the edits in question to make sure that I get them all.

@Dunbaratu
Copy link
Member

It might be okay to move them so that they don't break up the flow of the main discussion, but I did try to put them in roughly the same location as the thing they were talking about. Ideally I'd like to see the layout changed so they are just sidebars instead of interrupting the flow of text, but my CSS-fu isn't quite up to the task of working out how to meddle with Sphinx to make it do that.

I'll do a more complete review a bit later (I'm heading out the door).

Copy link
Member

@Dunbaratu Dunbaratu left a comment

Choose a reason for hiding this comment

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

I went through the whole thing and wrote "Agree" or "Disagree" on the edits to say if I agree with the deletion or disagree with the deletion.

doc/source/addons/AGX.rst Show resolved Hide resolved
doc/source/addons/IR.rst Show resolved Hide resolved
doc/source/addons/IR.rst Show resolved Hide resolved
doc/source/addons/IR.rst Show resolved Hide resolved
doc/source/addons/IR.rst Show resolved Hide resolved
doc/source/structures/vessels/vessel.rst Outdated Show resolved Hide resolved
doc/source/structures/vessels/vessel.rst Outdated Show resolved Hide resolved
doc/source/structures/vessels/vessel.rst Show resolved Hide resolved
doc/source/structures/vessels/vessel.rst Show resolved Hide resolved
@@ -30,9 +29,6 @@ Intermediate
:doc:`Design Patterns Tutorial <tutorials/designpatterns>`
Discusses some general aspects of kOS flow control and optimizations.

:doc:`PID Loop Tutorial <tutorials/pidloops>`
Starts with a basic proportional feedback loop and develops, in stages, a complete PID-loop to control the throttle of a simple rocket design.

Copy link
Member

Choose a reason for hiding this comment

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

Disagree. Are you saying the old pid loop tutorial should be completely removed? It's not broken or wrong. It still works, and teaches how a PID works. It just uses a homebrewed PID instead of the built-in one. One could argue that a new version needs to be written that uses the built-in PID structure, but that doesn't mean that it's wrong to explain what a PID loop is doing under the hood by making a homemade one from scratch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent here was to remove the old tutorial completely. I definitely think a new version which uses the built-in structure would be valuable, but I think there are other explanations of PID loops that can be referenced here, such as the Wikipedia entry https://en.wikipedia.org/wiki/PID_controller linked in the subreddit's sidebar.

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with this, but I figure the same PR that deletes the old explanation should also be the PR that adds the new alternative for it. Can you put this link reference in there to replace the old page? (I'm not a fan of "break something in this PR, and we'll repair it later in a second PR" because of how often one PR gets merged and the other one doesn't.)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally I'd like to write a new PID page that isn't as hard to read as most pages written by mathematicians, like the wiki page, are, but that can come later as long as the wiki link is put in here for now. The wiki link isn't any worse than the current page is, so go ahead and remove it and replace it with the wiki link.

The first pass removed some information that is helpful to folks running 
older scripts they find online.
@mathuin
Copy link
Contributor Author

mathuin commented Jan 10, 2019

I resolved all the comments where you agreed, and where you disagreed and I replaced things. There's still a few points left to address, though.

A number of the places where scripts might break due to changes in kOS could be addressed with deprecation warnings or exceptions from within kOS, but it might be too late to add that sort of thing for older features.

I've pushed all the changes I've made. There should be less to review. :-)

@Dunbaratu
Copy link
Member

Dunbaratu commented Jan 13, 2019

Thanks for all the work.

I went through and agreed with all the new edits, except for 1 - the pidloop tutorial should (IMO) at least be replaced with some kind of off-site link as you suggested, rather than completely deleted and not replaced with anything.

It can even be a small page that just has one sentence along the lines of "The topic of PID loops is complex and the kOS docs don't contain a good tutorial for it. Until they do, use this link to an offsite page describing the concept:...."

@mathuin
Copy link
Contributor Author

mathuin commented Jan 24, 2019

Actually, I think I'm going to keep the PID loop tutorial. Earlier this week I watched a new player grapple with a hand-rolled PID loop on Discord -- this tutorial would have been helpful.

The PID loops tutorial has value.
@Dunbaratu
Copy link
Member

I plan to merge this today. The only thing left before I do is that I'm going through a test first to make sure it doesn't have any markdown errors that break the doc-generating sphinx tool.

@Dunbaratu Dunbaratu merged commit d375061 into KSP-KOS:develop Jan 28, 2019
@mathuin mathuin deleted the remove-ancient-wisdom branch January 28, 2019 19:48
@mathuin mathuin mentioned this pull request Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants