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

Add QuickBASIC #7080

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

DecimalTurn
Copy link
Contributor

@DecimalTurn DecimalTurn commented Oct 6, 2024

Description

This PR adds QuickBASIC (QB) as a langauge for the .bas extension.

Note that there exists a modern version of QuickBASIC known as QB64 that is still compatible with Classic QB. Since Classic QB and QB64 haven't reached the popularity threshold by their own at the moment, I've kept them combined, but we could separate them in the future if popularity justifies it.

The grammar used is the one for QB64, since it is also compatible with Classic QB.

Checklist:

@DecimalTurn DecimalTurn requested a review from a team as a code owner October 6, 2024 19:50
@DecimalTurn
Copy link
Contributor Author

Related issue: #3216

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

See inline comment

- '^[ ]*(CONST|DIM|REDIM|DEFINT|PRINT|DECLARE (SUB|FUNCTION)|FUNCTION|SUB) '
# Otherwise, we default to VBA as long as there is a proper Sub or Function definition.
- language: VBA
pattern: '^[ ]*(Private |Public )?(Sub|Function) '
Copy link
Member

Choose a reason for hiding this comment

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

Please don't do this... you're defining VBA and QuickBasic multiple times in the same heuristic. This indicates your heuristics have scope for improvement. This is also confusing and will lead to issues in future.

/cc @Alhadis for some regex skills.

Copy link
Contributor Author

@DecimalTurn DecimalTurn Oct 8, 2024

Choose a reason for hiding this comment

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

As far as I know, there is no way to do what I'm trying to do (for VBA) without having more than one entry per language for the .bas extension. I assumed that it was a fair approach since this also occurs for the .properties extension and the .1in extension. However, I remain open to suggestions if there is a better way.

.properties (INI and Java Properties appear twice each):

- extensions: ['.properties']
rules:
- language: INI
and:
- named_pattern: key_equals_value
- pattern: '^[;\[]'
- language: Java Properties
and:
- named_pattern: key_equals_value
- pattern: '^[#!]'
- language: INI
named_pattern: key_equals_value
- language: Java Properties
pattern: '^[^#!][^:]*:'

.1in (Roff Manpage appears twice):

- extensions: ['.1in', '.1m', '.1x', '.3in', '.3m', '.3p', '.3pm', '.3qt', '.3x', '.man', '.mdoc']
rules:
- language: Roff Manpage
and:
- named_pattern: mdoc-date
- named_pattern: mdoc-title
- named_pattern: mdoc-heading
- language: Roff Manpage
and:
- named_pattern: man-title
- named_pattern: man-heading
- language: Roff

Copy link
Contributor Author

@DecimalTurn DecimalTurn Oct 8, 2024

Choose a reason for hiding this comment

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

For the second QuickBASIC entry, I could actually merge it with the first considering that VBA/VB6 uses the PascalCase convention and code that was edited in the VB Editor will never have uppercase capitalization for keywords due case enforcing (as discussed here).

Technically, you could have uppercase keywords if you edit VBA/VB6 code while using another editor and it would be converted seemlessly when added to a VB Project, but I haven't seen any cases of that on GitHub. However, GitHub Search is case-insensitive, so it's hard to check.

@DecimalTurn
Copy link
Contributor Author

DecimalTurn commented Oct 8, 2024

@lildude due to the potential interactions between my 2 other open PRs, would it be OK if I rebase them onto one another such that this one is at the top (while still being separate PRs)? This would be better for testing while this PR is being worked on and it should avoid any conflicts at merge time.

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.

2 participants