issue-#27 Adding SET conversion to Strings

Should we merge the changes of issue-#27 into master?

Yes
8
100%
No
0
No votes
Abstain
0
No votes
 
Total votes: 8

User avatar
Josef Templ
Posts: 2048
Joined: Tue Sep 17, 2013 6:50 am

issue-#27 Adding SET conversion to Strings

Post by Josef Templ »

This poll is for merging the changes made in branch issue-#27
(Adding SET conversion to Strings) into master.

see redmine issue http://redmine.blackboxframework.org/issues/27.

Gentlemen, please cast your vote.

- Josef
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#27 Adding SET conversion to Strings

Post by Ivan Denisov »

User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#27 Adding SET conversion to Strings

Post by Robert »

Sorry, I missed this discussion.

Can someone tell me how to read the proposed Mod & Docu text?

Does the proposal produce the same output as the current TextFormatters.WriteSet procedure?


Robert
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#27 Adding SET conversion to Strings

Post by Ivan Denisov »

Robert wrote:Can someone tell me how to read the proposed Mod & Docu text?
You can download test version (zipped) or view the diff link in my previous messages.
Robert wrote:Does the proposal produce the same output as the current TextFormatters.WriteSet procedure?
We thought about the same :)
I just checked, yes it produces the same output, so I am suggesting to modify the TextFormatters.WriteSet:

Code: Select all

	PROCEDURE (VAR f: Formatter) WriteSet* (x: SET), NEW;
		VAR str: ARRAY 128 OF CHAR;
	BEGIN
		Strings.SetToString(x, str);
		f.WriteString(str);
	END WriteSet;
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#27 Adding SET conversion to Strings

Post by Robert »

I am happy with the existing TextFormatters format, so that is OK. (Surely 128 characters is way overkill?)

Forgive me if the answer is obvious, but I haven't had time to look. Most String routines have a ToString and matching FromString version. Does this
proposal have compatible conversion both ways?

I find it convenient to have two styles for sets: the usual {1, 3, 5..6} style, and a hexadecimal style as you see in TRAP windows.

Does the proposal have a field width control input?
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#27 Adding SET conversion to Strings

Post by Ivan Denisov »

Robert wrote:Surely 128 characters is way overkill?
The string
" {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}0X"
contains 122 symbols... it will be reduced to " {0..31}0X", but I was thinking about some theoretical limit.
Robert wrote:Forgive me if the answer is obvious, but I haven't had time to look. Most String routines have a ToString and matching FromString version. Does this
proposal have compatible conversion both ways?
Yes, Doug checked this well.
Robert wrote:Does the proposal have a field width control input?
As I understood, no.
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#27 Adding SET conversion to Strings

Post by Robert »

Thanks for the detailed replies.

I have a rather more general question.

I thought that the changes being worked on were for centre version 1.7, and that its purpose was to incorporate bug fixes, and to avoid
having several competing versions of BlackBox being used at the same time.

This change feels like an added feature, not a bug fix.

While I am not arguing that this SET example is a bad feature, should it go into the first centre release?

And do we not need to draft a careful policy about adding new features before it goes into any release?
It is difficult to find the "right" balance between adding useful stuff and drifting into unmaintainable bloatware. This is a big question
that we have not started to discuss yet.

Regards
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#27 Adding SET conversion to Strings

Post by Robert »

A couple of comments / questions on the Docu file:

1 - SetToString - An example or two would be good. Are there spaces after "," & ".."?

2 - StringToSet - In the syntax is "dec" defined?
- dec I am guessing is a number. Do the decs need to be strictly increasing?
- Is white space in the input permissable?


Cheers, Robert
User avatar
Josef Templ
Posts: 2048
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#27 Adding SET conversion to Strings

Post by Josef Templ »

> This change feels like an added feature, not a bug fix.

This issue is clearly marked as a 'Feature' in the redmine issue tracker.
see http://redmine.blackboxframework.org/pr ... box/issues.
Adding SET conversions closes a gap in the Strings modules.
Now it is possible to convert any non-trivial primitive data type.
It also saves some code in TextMappers.Formatter.WriteSet.

For converting a SET s to a hex number you can use Strings.IntoToStringForm(ORD(s), ...).

I have added the support of white space in the docu. Thanks for the hint.

'dec' is a positive decimal integer number. I have also added this in the docu.

I have also added an example for SetToString conversion.

I was not aware of TextMappers.Formatter.WriteSet when I tried to simplify SetToString.
I should have looked into it, though. It is a much better formulation because it
is faster for small sets, which are quite common. I have used that as the foundation
for Strings.SetToString.

Thanks everybody for the feedback. Better late than never!
Re-voting is allowed, by the way. In addition, I will certainly not close the voting until the
discussion is finished.

- Josef
Last edited by Josef Templ on Sun Feb 08, 2015 9:56 pm, edited 1 time in total.
cfbsoftware
Posts: 204
Joined: Wed Sep 18, 2013 10:06 pm
Contact:

Re: issue-#27 Adding SET conversion to Strings

Post by cfbsoftware »

Ivan Denisov wrote: contains 122 symbols... it will be reduced to " {1..32}0X"
The correct set range for the 1.6 version of BlackBox is {0..31}. Is that what is supported in this change?
Locked