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

Update CommunityTechTree.netkan to state with Nertea seal-of-approval #1565

Merged
merged 2 commits into from
Jun 13, 2015
Merged

Update CommunityTechTree.netkan to state with Nertea seal-of-approval #1565

merged 2 commits into from
Jun 13, 2015

Conversation

lOmicronl
Copy link
Contributor

No description provided.

@Dazpoet
Copy link
Member

Dazpoet commented Jun 10, 2015

Should this conflict with OpenTree?

@dbent
Copy link
Member

dbent commented Jun 11, 2015

Please clean up the supports relationships in this and other PRs for the reasons specified in my post here.

In addition:

  • It's counter to how existing mods use the relationship.
  • It forces information not specific to the mod into the mod's metadata. What happens when a new mod adds support for CTT? Do we update CTT's metadata and force an update when nothing about CTT itself has changed?
  • It's redundant with the inverse depends, recommends, suggests, or supports relationships of the other mods.
  • It will hamper efforts to make supports more than an informational field in the future.

Your point about CKAN not showing inverse relationships is well taken and I've submitted KSP-CKAN/CKAN#100 which I may implement myself.

@pjf
Copy link
Member

pjf commented Jun 11, 2015

For what it's worth, supports is an "information only" field as defined by the spec, which means we might show it to users, but we'll never use it to decide what to install, or not to install. Supports essentially says "X works well with Y", and the CTT definitely works well with all the mods in the support list. :)

However since we've seen two interpretations of supports, this implies that we really need to clarify the spec on this point. I've opened KSP-CKAN/CKAN#1104 for this purpose. My opinion is that this PR looks pretty good as it is, and if we're forcing users to update if the supports (or any other information-only) field changes, then that's a bug (or at least misfeature) in our client.

@lOmicronl
Copy link
Contributor Author

@Dazpoet Indeed it should. It escaped my search for other tech trees because it does not have the word "tech" in its name, which is the keyword I looked for. Once I figure out how to edit an already pending pull request, I will change that.

But perhaps it's best to leave the pull request open another day or two anyway, until pjf's discussion on clarifying the spec on the topic of the 'supports' relationship bears fruit. Once I know into which direction things are heading, I'll know whether to remove, change or leave the current entries in that field. CTT is currently working just fine anyway, it doesn't particularly need this update except for information purposes (and perhaps noting the conflict).

@lOmicronl lOmicronl closed this Jun 11, 2015
@lOmicronl lOmicronl deleted the patch-1 branch June 11, 2015 20:06
@lOmicronl lOmicronl restored the patch-1 branch June 11, 2015 20:06
@Dazpoet
Copy link
Member

Dazpoet commented Jun 11, 2015

Seems ETT will be added to CKAN pretty soon aswell #1581

@lOmicronl
Copy link
Contributor Author

Noted.

Also I completely pressed a wrong button and accidentally closed this... let's see what this button here does...

@lOmicronl lOmicronl reopened this Jun 11, 2015
@pjf
Copy link
Member

pjf commented Jun 12, 2015

Also I completely pressed a wrong button and accidentally closed this... let's see what this button here does...

Today I learned there's a re-open button for pull requests. (I swear that didn't exist a couple of years ago!)

Removed 'supports' block, added 'conflicts' block.
@lOmicronl
Copy link
Contributor Author

I took out the 'supports' block for now, and added conflicts for all tech trees I could find. Should be okay to pull in now.

Dazpoet added a commit that referenced this pull request Jun 13, 2015
Update CommunityTechTree.netkan to state with Nertea seal-of-approval
@Dazpoet Dazpoet merged commit 6cd5b91 into KSP-CKAN:master Jun 13, 2015
@lOmicronl lOmicronl deleted the patch-1 branch June 13, 2015 21:20
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.

4 participants