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

$VERSIONINFO enhancement (#20) #392

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

a740g
Copy link
Contributor

@a740g a740g commented Oct 9, 2023

This PR implements $VERSIONINFO enhancement as described in #20.

This works in a backwards compatible way which allows the old behavior but also suggests the user (via warnings) to enclose the strings using the string bracket delimiter ''.

  • Allows old behavior
  • FILEVERSION# and PRODUCTVERSION# are exempted
  • Honors warning setting - Options > Ignore Warnings

Warning messages for current qb64pe.bas $VERSIONINFO:
image

Closes #20.

@a740g a740g added the enhancement New feature or request label Oct 9, 2023
@a740g a740g self-assigned this Oct 9, 2023
@RhoSigma-QB64
Copy link
Member

RhoSigma-QB64 commented Oct 9, 2023

Shouldn't this PR not do the required adjustments in qb64pe.bas to avoid those warnings?

@a740g
Copy link
Contributor Author

a740g commented Oct 9, 2023

Shouldn't this PR not do the required adjustments in qb64pe.bas to avoid those warnings?

We can wrap the $VERSIONINFO strings in quotes in q64pe.bas so that it does now show these warnings. Even if we leave them the way they are, nothing will break.
I am ok adding the quotes if everyone is ok. It'll save another PR down the line.

Did I even understand the question correctly? 🙂

@RhoSigma-QB64
Copy link
Member

RhoSigma-QB64 commented Oct 9, 2023

For me it's unclear why we need to throw warnings at all, if both methods (w/ and w/o quotes) are valid and allowed? But, if we want warn about missing quotes, then we should clean our own house, making sure we don't get warnings on our own code. Just a matter of my sick perfectionism I guess :)

@a740g
Copy link
Contributor Author

a740g commented Oct 9, 2023

For me it's unclear why we need to throw warnings at all, if both methods (w/ and w/o quotes) are valid and allowed? But, if we want warn about missing quotes, then we should clean our own house, making sure we don't get warnings on our own code. Just a matter of my sick perfectionism I guess :)

Ah. Ok. I get it. And you are right.

I added the warnings because that's what #20 suggested. And TBH, adding warnings does seem a little useful. Without the quotes the IDE attempts to syntax-highlight the string which ends up looking ugly. The warning basically motivates the user to add the quotes.

Fun fact: When I first started using QB64, I always used to add the quotes to the $VERSIONINFO strings thinking that was a natural thing to do.

I am good both ways:

  1. We can drop the warnings completely, or
  2. We just add the quotes to clean our own house

@RhoSigma-QB64
Copy link
Member

Without the quotes the IDE attempts to syntax-highlight the string which ends up looking ugly. The warning basically motivates the user to add the quotes.

Yeah, noticed that too when implementing the code export stuff, but it's rather a matter of finetuning the syntax highlighter then. I already did that for the code export to avoid this faulty highlighting. I can look at the IDE highlighter later, for now I'd say just clean our house :)

@a740g
Copy link
Contributor Author

a740g commented Oct 9, 2023

Without the quotes the IDE attempts to syntax-highlight the string which ends up looking ugly. The warning basically motivates the user to add the quotes.

Yeah, noticed that too when implementing the code export stuff, but it's rather a matter of finetuning the syntax highlighter then. I already did that for the code export to avoid this faulty highlighting. I can look at the IDE highlighter later, for now I'd say just clean our house :)

Done.

@RhoSigma-QB64
Copy link
Member

RhoSigma-QB64 commented Oct 9, 2023

BTW in #20 Matt mentioned that by his knowlegde $VERSIONINFO was the only Meta-Command which allows unquoted strings, but I know at least one other, it's $ERROR, maybe you want to look at that too and fix it in the same run.

What's about $LET, can we define a string such as $LET me = RhoSigma? Must admit I avoid the pre-compiler till now.

@a740g
Copy link
Contributor Author

a740g commented Oct 10, 2023

BTW in #20 Matt mentioned that by his knowlegde $VERSIONINFO was the only Meta-Command which allows unquoted strings, but I know at least one other, it's $ERROR, maybe you want to look at that too and fix it in the same run.

What's about $LET, can we define a string such as $LET me = RhoSigma? Must admit I avoid the pre-compiler till now.

Fair callout on $ERROR. I'll get that to be compliant as well.
I usually use $LET to create per-processor variables and seldom assign any string values. It however requires strings to be properly quoted using " if you are assigning one.
While on the topic, $MIDISOUNDFONT seems to require " instead of '. I am not sure if this is intentional and requires any change. 🤷‍♂️

@a740g a740g closed this Oct 14, 2023
@a740g a740g force-pushed the versioninfo-enhancement branch from e7aecc6 to 1d95c73 Compare October 14, 2023 00:54
@a740g a740g reopened this Oct 14, 2023
@a740g
Copy link
Contributor Author

a740g commented Oct 14, 2023

Enabled $ERROR to handle strings enclosed using '' as suggested by @RhoSigma-QB64.

@RhoSigma-QB64
Copy link
Member

With your new FUNCTION RemoveStringEnclosingPair() you may want to check its usability for '$INCLUDE: 'file.bm' too have not several different handlings for several meta-commands.

@a740g a740g requested review from SteveMcNeill and mkilgore and removed request for SteveMcNeill and mkilgore October 14, 2023 18:42
@a740g
Copy link
Contributor Author

a740g commented Oct 15, 2023

With your new FUNCTION RemoveStringEnclosingPair() you may want to check its usability for '$INCLUDE: 'file.bm' too have not several different handlings for several meta-commands.

I looked at the $INCLUDE handling here and it seems like there is special handling been done due to this metacommand being embedded in a comment and some QB45 behavior. It does mandate usage of ' to enclose file names. This is part of the giant lineformat$() function. IMO, it is best we leave it the way it is and re-visit it sometime in the future as part of a significant compiler re-write / simplification.

@a740g a740g merged commit 00ae109 into QB64-Phoenix-Edition:main Oct 15, 2023
@a740g a740g deleted the versioninfo-enhancement branch October 15, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$VERSIONINFO should accept a quoted string
2 participants