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

RCHIP-01: Add bytesToHex method #11

Open
SteveHenley opened this issue May 15, 2020 · 14 comments
Open

RCHIP-01: Add bytesToHex method #11

SteveHenley opened this issue May 15, 2020 · 14 comments
Labels
Approved Approved proposal Implemented Technical Technical Proposal

Comments

@SteveHenley
Copy link
Collaborator

Specify expressions for string (Aurthur Greef PR merge - convert into RChip)
Dappy needs Scala regular expressions to be exposed in Rholang - e.g. to check for domain names

https://www.tutorialspoint.com/scala/scala_regular_expressions.htm

@SteveHenley SteveHenley changed the title RChip 09 Improve string function and exposing Scala regular expressions RChip 09 - Improve string function and exposing Scala regular expressions May 15, 2020
@SteveHenley
Copy link
Collaborator Author

The original intent of this RChip 09 was to address the implementation of bytesToHex.
Does the scope of this RChip need to be narrowed so that it specifically addresses bytesToHex?

Add bytesToHex method #2918

@tgrospic
Copy link
Collaborator

@SteveHenley bytesToHex is only applicable to Rholang byte array so it should not be mixed with additional methods for regular expressions on strings.

@dckc
Copy link

dckc commented May 20, 2020

I see this is cited by rchain/rchain#2918 please reduce the scope and update the title for bytesToHex only, @SteveHenley .

Also, this seems to be missing much of the information required by the RCHIP draft from the 2018 annual meeting:

  • Category for Prioritization
  • Score for Category from 1 → 10 (1 being lowest, 10 being greatest)
  • Metrics to measure success (specific goal that justifies the score)
  • Detailed description of the feature & benefits
    • Does the feature open up new markets, if so how?
    • Are users blocked from doing something without the feature? Or is this a UX change?
    • Are there contractual obligations related to the feature? (This alone won't bump priority. Contracts should never be signed against un-implemented features)

I don't suppose that was ever ratified, but I haven't seen anything that supersedes it.

Who is on the Editorial Committee? the Approval Committee?

p.s. for follow-up on process, see #14

@SteveHenley
Copy link
Collaborator Author

To clarify, if you are request that the title for RChip 09 be updates to byesToHex only, then here is Rao's reply:
"bytestoHex is one of the string functions. So we need only one. Exposing Scala regular expressions is the current agreed to scope of 'Improve string functions'".
https://canary.discordapp.com/channels/375365542359465989/393462637100400650/711016913186324529

There is currently no editorial committee nor approval committee. Due to the limited manpower the tech governance working group decided to go with lighter version of the RChip. Rao can better explain what this lighter version is. @dckc

@dckc
Copy link

dckc commented May 21, 2020

rchain/rchain#2918 doesn't cover Exposing Scala regular expressions. It's just bytesToHex. For sanity's sake, please either expand the scope of the PR or narrow the scope this issue. The latter sure seems easier.

As to "agreed scope", please cite the decision record. I don't believe anyone has mandate to make such decisions on behalf of the coop or even on behalf of me.

@jimscarver
Copy link
Collaborator

To avoid ReDoS attacks and pricing issues with regular expressions we can provide basic pattern matching by supporting unix file glob patterns. https://en.wikipedia.org/wiki/Glob_(programming)

Regular expression might best be implimented as rholang patterns rather than strings in a separate RChip

@jimscarver
Copy link
Collaborator

jimscarver commented May 21, 2020

Given the lack of full regular expression support a frequent requirement is for tolowercase(String) so that rchain can support case insensitive strings and "Jim" tolowercase can be compared to be equal to "jim".

@SteveHenley SteveHenley changed the title RChip 09 - Improve string function and exposing Scala regular expressions RChip-09-Improve string function: bytesToHex May 25, 2020
@SteveHenley
Copy link
Collaborator Author

As a result of the last tech governance session discussionsI have updated the title of RChip-09 to Improve string function: bytesToHex so that it is more narrow in focus.

@SteveHenley SteveHenley changed the title RChip-09-Improve string function: bytesToHex RChip-09-bytesToHex May 31, 2020
@SteveHenley
Copy link
Collaborator Author

Update title to be more concise based comments above and tech governance discussion.

@SteveHenley
Copy link
Collaborator Author

I have created a google docs RChip-09-bytesToHex for us to collaborate.
The doc uses two templates, at the top there is "Specify the Features" which is from the original RChip process. At "History" we have the second template which the SIP. This is our first attempt at designing a template we want to use. The template we standardize on will eventually get narrowed down.

@tgrospic tgrospic added the Approved Approved proposal label Jun 4, 2020
@9rb 9rb added the Technical Technical Proposal label Jun 4, 2020
@9rb 9rb changed the title RChip-09-bytesToHex RChip-01-bytesToHex Jun 4, 2020
@9rb 9rb changed the title RChip-01-bytesToHex RCHIP-01-bytesToHex Jun 4, 2020
@9rb 9rb changed the title RCHIP-01-bytesToHex RCHIP-01: bytesToHex Jun 4, 2020
@9rb 9rb changed the title RCHIP-01: bytesToHex RCHIP-01: Add bytesToHex method Jun 4, 2020
@dckc
Copy link

dckc commented Jun 23, 2020

@tgrospic I just noticed the Approved label here; that's not a judgement you made unilaterally, right? I suspect the decision was made by the tech governance group. Please cite the record of that decision. (Or make a record of it here, noting when it was made and who participated. Enumerating yes / no / abstain is traditional too)

@tgrospic
Copy link
Collaborator

@dckc I added label Approved after PR was merged. I agreed to approve the PR after decision on the tech governance group. We decided to be less demanding on the proposal description because working code is already created.
I'm not aware that we have written record. @SteveHenley, @9rb can you help me here?

I suggested that we use emoticons (:+1: :-1:) in the description (?).

@SteveHenley
Copy link
Collaborator Author

@dckc Please refer to the Log: Tech Governance, date 2020-06-25, agenda item #4 regarding the approval of RCHIP-01: Add byesToHex Method and all subsequence RCHIPs and RCHIP issues.

@dckc
Copy link

dckc commented Jun 26, 2020

OK. If everything is approved, I trust you'll get rid of the Approved label, since it doesn't mean anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Approved proposal Implemented Technical Technical Proposal
Projects
None yet
Development

No branches or pull requests

5 participants