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

Separate Visual Basic into VB.NET, VBA/VB6 and VBScript #4725

Merged
merged 14 commits into from
Dec 5, 2019

Conversation

zspitz
Copy link
Contributor

@zspitz zspitz commented Nov 25, 2019

Description

See #2418 .

Checklist:

@zspitz
Copy link
Contributor Author

zspitz commented Nov 26, 2019

Per this suggestion I am attempting to create a PR. However, I am running into the following issues:

  1. I can't get a working Ruby install on my Windows box. This means I can't update the language ids, and update the grammar.yml file. It also means I have no way of confirming that tests actually succeed, short of committing and waiting for the GitHub actions to complete. (NB. I have no experience with Ruby, but if I had a working install I would manage somehow.) My next step is a Linux VM on a different Windows machine, but it would be helpful if there is some way to bypass this.

  2. The languages.yml file is supposed to allow an entry as follows:

# fs_name               - Optional field. Only necessary as a replacement for the sample directory name if the
#                         language name is not a valid filename under the Windows filesystem (e.g., if it
#                         contains an asterisk).

In this case, it would be useful for the VBA/VB6 language entry, containing a /, which is an invalid Windows path character; so I've specified fs_name: VBA_VB6. But the test run still expects the samples to be in a folder called VBA/VB6, and fails on this.

@zspitz zspitz mentioned this pull request Nov 26, 2019
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.

I can't really comment on installing Ruby on Windows, but I can if you encounter problems with your Linux VM approach.

I've made a few suggestions that will help things along.

lib/linguist/languages.yml Outdated Show resolved Hide resolved
lib/linguist/languages.yml Outdated Show resolved Hide resolved
lib/linguist/languages.yml Outdated Show resolved Hide resolved
@cbeauchesne
Copy link

@zspitz actually, VBA is VB6, it's the VB6 runtime used in dev mode (binary include debugger symbols, that allow the famous yellow line). It's exactly the mode used in the VB6 editor when you develop.

MS simply included the VB6 IDE inside Office Apps. For instance, you can see in VBA a legacy compile method on VBProject object, but calling it will raise an exception : "not supported for this project".

So, you will never find something in VBA that does not exists in VB6, and IMO, you should use VB6 for fs_name.

@zspitz
Copy link
Contributor Author

zspitz commented Nov 26, 2019

@cbeauchesne

@zspitz actually, VBA is VB6, it's the VB6 runtime used in dev mode (binary include debugger symbols, that allow the famous yellow line). It's exactly the mode used in the VB6 editor when you develop.

MS simply included the VB6 IDE inside Office Apps. For instance, you can see in VBA a legacy compile method on VBProject object, but calling it will raise an exception : "not supported for this project".

Not disagreeing, but I think that (as I said) VBA is far more commonly used than VB6, so it makes more sense that the language name should be VBA (or Visual Basic for Applications), rather than VB6.

So, you will never find something in VBA that does not exists in VB6, and IMO, you should use VB6 for fs_name.

The reverse is also true to my knowledge -- in terms of the source of the language itself (not the global objects, or the runtime environment), you won't find anything in VB6 not in VBA

@zspitz zspitz requested a review from lildude November 27, 2019 05:04
@zspitz
Copy link
Contributor Author

zspitz commented Nov 27, 2019

@lildude

RE VBA vs Visual Basic for Applications -- The formal name isn't always used, or even preferred: e.g. HTML, CSS, XML, CSV; even when the acronym is not universally known to people unfamiliar with the language (such as GAMS, HLSL, VHDL). Therefore, can we keep the name as VBA?

I've updated the ids, but other tests are still failing.

@cbeauchesne
Copy link

cbeauchesne commented Nov 27, 2019

The reverse is also true to my knowledge -- in terms of the source of the language itself (not the global objects, or the runtime environment), you won't find anything in VB6 not in VBA

Partially true :) It's the same runtime, but you'll find several things not possible, or hardly doable in VBA. I've in mind options for default immutable value for objects (x = range.value is equivalent to x = range for excel range, you can do the same for your objects), and object iterators (for each item in myObject). On VB6, it's a options in the IDE, and it's saved as a header of your .cls file. In VBA, the option is missing in the IDE, and if you export the file, you'll never find the header. But, if you import a cls file with the good header, you can add this behavior (and if you modify your file in VBA IDE, you loose your hack :( ).

I used to build my own build tooling for VBA for this use-case.

That said, I agree that's very small differences, and as VBA usage is far more common than VB6, the less common surprise effect is probably a better argument.

@lildude
Copy link
Member

lildude commented Nov 28, 2019

Therefore, can we keep the name as VBA?

Yeah, you can keep VBA. I was more trying to draw things away from the previous attempt of VBA/VB6 whilst keeping things unambiguous. I think it's commonly known that VBA is referring to Visual Basic for Applications as opposed to any other language.

@zspitz
Copy link
Contributor Author

zspitz commented Dec 3, 2019

@lildude Is there anything I can add/change to move this forward? Thanks.

@lildude lildude dismissed their stale review December 4, 2019 09:34

Changes implemented.

@lildude
Copy link
Member

lildude commented Dec 4, 2019

@lildude Is there anything I can add/change to move this forward? Thanks.

Not from what I can see. Just waiting on a review from one of the other maintainers before I make my final review and merge.

@lildude lildude requested review from pchaigno and Alhadis December 4, 2019 09:35
Copy link
Collaborator

@Alhadis Alhadis left a comment

Choose a reason for hiding this comment

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

Looks okay to me. I'm not familiar with Visual Basic, so I'll take @zspitz's word that the split is justified.

@lildude lildude merged commit 7fa8ca2 into github-linguist:master Dec 5, 2019
@zspitz
Copy link
Contributor Author

zspitz commented Dec 5, 2019

Out of curiosity, at what point will this be visible in existing repos? I understand from the Linguist README that GitHub only performs this analysis when code is pushed to a repo. Is there ever another time when this happens?

@lildude
Copy link
Member

lildude commented Dec 5, 2019

Out of curiosity, at what point will this be visible in existing repos? I understand from the Linguist README that GitHub only performs this analysis when code is pushed to a repo. Is there ever another time when this happens?

The rename should take effect in some places when I make the next release, but only for "Visual Basic" -> "Visual Basic .NET" rename as the language ID is the same. Everything else will only take affect on those repos on the next push. The reason I say "some places" is because different parts of the site are cached differently and I have no idea how they all tie in.

@lildude
Copy link
Member

lildude commented Dec 6, 2019

These changes are now live:

Screenshot 2019-12-06 at 16 43 31

Anything not correctly reflected will need a push, and if that doesn't work, contact GitHub support as something else will be caching the old name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants