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

CIP-0035 | Update for new CIP-1 #437

Merged

Conversation

michaelpj
Copy link
Contributor

This updates CIP-35 to be in line with the new CIP-1.

The main changes are:

  • Remove some sections that are IMO implied by CIP-1, e.g. the necessity of a clear implementation plan.
  • Delete the CIP registry, since you can just list the category
  • Explicitly identify the 'Plutus' category and list the reviewers. At the moment it's just me, maybe it should be more people?
  • Attempt to clarify the prose a bit.

There's one very questionable thing that I've done, which is to assert that CIPs in the crossover between the Ledger and Plutus should be in both categories. I just went ahead and wrote that here, but I defer to the resolution of #433 and I'll change this depending on what gets decided there.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@michaelpj this is great and also helps for compliance with the new standardisation proceeding via #389. At first view the document structures all look parseable (headers at the right levels) but the new form also needs:

a Path to Active for each one, even ones that are currently Active.

a section at the bottom with a link to the License quoted in the header (editors can edit these in later if missed here).

another nitpick which we might also handle in concierge style: each of the Motivation and Rationale sections now have slightly longer titles:

  • Motivation: why is this CIP necessary?
  • Rationale: how does this CIP achieve its goals?

... @KtorZ I feel sheepish asking people to add these as the literal section titles, especially considering what it does to the section anchors (instead of simple & immutable anchors like #motivation), but these are mandatory section titles as per https://github.com/cardano-foundation/CIPs/tree/master/CIP-0001#structure.

Comment on lines 5 to 6
Comments-Summary: No comments
Comments-URI:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This CIP preamble needs the same makeover as CIP's 31 to 33 (some obsolete fields here). Note we will also be double checking this in #389.

Comment on lines 9 to 11
Categories:
- Plutus
- Ledger
Copy link
Collaborator

Choose a reason for hiding this comment

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

The duplicate category seems perfectly understandable to me but this syntax still awaits confirmation via issue #433 and probably also (@KtorZ) an update to https://github.com/cardano-foundation/CIPs/tree/master/CIP-0001#categories.

Copy link
Member

Choose a reason for hiding this comment

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

There's actually no Ledger category.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, it still is not registered. We've added it for some existing CIPs; but I would refrain to add it to any new ones..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming there will be one, though.

Copy link
Member

Choose a reason for hiding this comment

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

I've been assuming for a couple of months too now 🙃

Copy link
Collaborator

@rphair rphair Feb 7, 2023

Choose a reason for hiding this comment

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

just noting for cross-reference: there is soon going to be one 🙂 #456

@rphair rphair requested a review from KtorZ January 17, 2023 14:38
@michaelpj
Copy link
Contributor Author

Thanks, I'll fix those things.

I can add the "Path to Active" although I'm not sure what I'd say. By its nature this CIP is Active in virtue of us endorsing it, so... what?

@rphair
Copy link
Collaborator

rphair commented Jan 17, 2023

If sticking to the guidelines as I understand them, maybe something like (edited to your satisfaction):


Acceptance Criteria

Agreement upon this CIP by the Plutus team is confirmation of its acceptance.

Implementation Plan

Implementation of the CIP proceeds as a matter of course after the Plutus team agrees upon it.


If it sounds dumb to have these tautological paths-to-active then maybe @KtorZ we could allow for its omission in certain cases... 🤔

@rphair rphair changed the title CIP-35: update for new CIP-1 CIP-0035 | Update for new CIP-1 Jan 17, 2023
@rphair
Copy link
Collaborator

rphair commented Jan 17, 2023

  • Motivation: why is this CIP necessary?
  • Rationale: how does this CIP achieve its goals?

@michaelpj since you were also editing CIP-0001 when the longer titles were assigned to these 2 sections maybe you could please also contribute your opinion about the latter part of #439.

@KtorZ
Copy link
Member

KtorZ commented Jan 18, 2023

@michaelpj By its nature this CIP is Active in virtue of us endorsing it

I'd be happy to see just this as a path to active :)

@michaelpj
Copy link
Contributor Author

I've addressed the comments, and removed the multiple categories. I've attempted to say something sensible about what to do in cases where things fall in the overlap, but we can fix it up later if it proves problematic I guess.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

thanks @michaelpj - this all looks both compliant & well explained to me.

@KtorZ are we strictly requiring that the H1 document titles contain the CIP-XXXX identifier as well?

  • I ask mainly because they've been edited into CIPs 2 through 4 (and I followed suit with 13) in the course of Update older CIPs for revised CIP-0001 #389.
  • It seems to me this could be left up to the author considering the number will be in the published URL and also just above in the header.

Sorry for another nitpick from the format nanny, but also trying to make sure whatever CI we might invent to normalise the presentation doesn't have to be more like an AI. 🤓

@KtorZ
Copy link
Member

KtorZ commented Jan 19, 2023

@KtorZ are we strictly requiring that the H1 document titles contain the CIP-XXXX identifier as well?

Perhaps not. I am on the fence with that. It reads better on Github, but it's currently redundant on the website (see e.g. https://cips.cardano.org/cips/cip1/). Plus, there's the "problem of section heading" that you outlined in #439 ... which makes me think we might just consider dropping it and bumping all section headers to h1 level 🤷

@rphair
Copy link
Collaborator

rphair commented Jan 19, 2023

dropping it and bumping all section headers to h1 level

Section headers as H1's is against the SEO point of view in which H1 is synonymous with the page title... while H2's for section headers is perfect. The problem with the current web site rendering is the duplicated H1 title.

So the problem is solved by eliminating the H1 title in the CIP text (allowing the web script to generate the H1 title) but leaving all the section headers as H2 (or making them that way).

@michaelpj any further formatting discussions can go back to #439 and if @KtorZ agrees with the above then your document formatting is fine the way it is... since if there's an extra H1 in the GitHub text then it'll easy to filter out in the web site generator.

@michaelpj
Copy link
Contributor Author

Okay, sounds like you're happy with this overall then?

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Yes 😎

@rphair rphair mentioned this pull request Jan 20, 2023
@KtorZ KtorZ merged commit e6cf475 into cardano-foundation:master Jan 20, 2023
Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
* Update CIP-35 to be more in line with the new CIP-001

* Update metadata for CIP-(31-33)

* Address comments
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.

None yet

3 participants