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 line numbers of cardano-serialisation-lib #275

Merged
merged 2 commits into from
Jul 5, 2022

Conversation

thisHermit
Copy link
Contributor

No description provided.

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.

Looks accurate. The current links on CIP-0008 don't point anywhere in particular in that file.

So approving as is but @SebastienGllmt maybe there's a more general way to refer to these code segments that will still work even if the code shifts around some more?

@rphair rphair requested a review from KtorZ June 16, 2022 17:18
@thisHermit
Copy link
Contributor Author

I see a way to point to a particular commit. Would that be an acceptable improvement?

@SebastienGllmt
Copy link
Collaborator

Github allows generating permalinks for files which will be more stable than the change you made

@rphair
Copy link
Collaborator

rphair commented Jun 28, 2022

I only know of linking to a specific commit, which will also also change (along with the line number) when the code is updated. What's the secret, @SebastienGllmt ... or is it best just to link to the file itself?
https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-a-permanent-link-to-a-code-snippet

@rphair
Copy link
Collaborator

rphair commented Jun 28, 2022

never mind that last comment... yes @thisHermit I think linking to the commit & line number would be fine since you're only trying to show that section of code, not necessarily its latest content (I was just hoping someone might know how to add some other type of anchor to search in the file, e.g a regex)

@rphair rphair added the Waiting for Author Proposal showing lack of activity or participation from their authors. label Jun 28, 2022
@KtorZ KtorZ added Correction Fixing minor issue or typo Waiting for Author Proposal showing lack of activity or participation from their authors. and removed Waiting for Author Proposal showing lack of activity or participation from their authors. labels Jun 29, 2022
1) [sign](https://github.com/Emurgo/cardano-serialization-lib/blob/master/rust/src/crypto.rs#L260)
2) [verify](https://github.com/Emurgo/cardano-serialization-lib/blob/master/rust/src/crypto.rs#L303)
1) [sign](https://github.com/Emurgo/cardano-serialization-lib/blob/4792b1b121e728a51686d5fcbffd33489d65c903/rust/src/crypto.rs#L279)
2) [verify](https://github.com/Emurgo/cardano-serialization-lib/blob/4792b1b121e728a51686d5fcbffd33489d65c903/rust/src/crypto.rs#L322)

You can see an example of these two functions [here](https://repl.it/repls/FlusteredSimpleFunction)
Copy link
Collaborator

@rphair rphair Jun 30, 2022

Choose a reason for hiding this comment

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

@SebastienGllmt is there an example somewhere that doesn't use Replit anonymously, so the code (pointed to on line 127) can be viewed without forking the repl?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no I don't know any good alternative

@rphair rphair removed the Waiting for Author Proposal showing lack of activity or participation from their authors. label Jun 30, 2022
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

I think the issue with repl.it is orthogonal to that PR. So, happy to merge this change now, and perhaps move the repl.it discussion to github issues

@KtorZ KtorZ merged commit 1ef8e81 into cardano-foundation:master Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Correction Fixing minor issue or typo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants