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

Class / Module Attributes lost on import #230

Closed
hecon5 opened this issue May 26, 2021 · 32 comments
Closed

Class / Module Attributes lost on import #230

hecon5 opened this issue May 26, 2021 · 32 comments
Assignees
Milestone

Comments

@hecon5
Copy link
Contributor

hecon5 commented May 26, 2021

I've noticed that when I have attributes in my modules, they're lost when imported.

Specifically, default member (Attribute FunctionName.VB_UserMemId = 0 is lost, as are any descriptions.

They do not show up when exported, either.

See BA – Attributes for what "should" be happening,

HERE and RD's Example Use are a few more examples.

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

Note: when I manually import, it appears to load just fine, and keeps the attributes; when using the addin, they're lost.

@joyfullservice
Copy link
Owner

When you do the manual import are you using the LoadFromText or the VBE import? You might do some testing to ensure that the Access export/import functions SaveAsText/LoadFromText support the hidden attributes. If they don't we might have to import them through VBE, or see if Access would preserve the attributes if we combined the VBE export with the .bas object source file and used LoadFromText to import the hybrid file.

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

Ok, some more troubleshooting.

When I manually import, I need the following added so that it is correctly loaded into the IDE as a class and not a regular module:

VERSION 1.0 CLASS
BEGIN
  MultiUse = -1  'True
End
Attribute VB_Name = "clsSillyName"
Attribute VB_GlobalNameSpace = False
Attribute VB_Creatable = False
Attribute VB_PredeclaredId = False
Attribute VB_Exposed = False
Option Compare Database

Which will break on import, and it is subsequently loaded as a "plain" module, and not a class.

As I poke around, I see there's not a separate clsDBClass like there is for clsDBModule. I'm wondering if there's something here.

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

When you do the manual import are you using the LoadFromText or the VBE import?

Good question: because I'm in a hurry, I just used the IDE; I'll give loadfromtext a go here now.

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

Well, did loadfromText and it doesn't appear to load properly; loads as a plain module and ignores the default member. I poked around in the importer, but I can't seem to figure out what's going on with the class/module determination on loadfromtext, or if that's done later.

It does appear to respect the Attribute VB_Name = "clsSillyName" attribute, but not the rest.

@joyfullservice
Copy link
Owner

Access uses the same object collection to export standard and class modules, and automatically figures out which type to use when importing the source file. This add-in project, for example, uses both standard and class modules, and they are always built correctly. My hunch is that the leading attribute lines is what signals LoadFromText to use a class module for the imported code.

If you would like to create a minimal database that demonstrates the issue, I could probably do a little testing on my end as well. I would like the add-in to support the hidden attributes. I am familiar with them, but haven't really used them in my projects.

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

Sure thing; it actually works for a PR I was going to make for clsConcat; I'll tidy that up and send it your way :)

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

see #232

@hecon5 hecon5 changed the title Attributes lost on import Class / Module Attributes lost on import May 26, 2021
@joyfullservice
Copy link
Owner

Thanks for the further details on this. I was able to reproduce the issue on my end. The export file includes the attributes, but they seem to be lost on import. I have some ideas on how to work with this...

@joyfullservice
Copy link
Owner

As a side note, there does not appear to be any way to import an object class module to an existing object using VBE, so this technique could not be used with forms or reports. That being said, we should be able to do this for class modules by overlaying a VBE import after importing the object.

I think I will also add this as a non-default option since it will involve slightly more overhead when checking for changes since changes to attributes are not reflected in the source code.

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

This is a good idea; I don't think that Forms/reports can even have Attributes in the same way that classes/modules can, so that makes sense to me, and seems the sanest/easiest methodology.

I wonder if a sort of reverse sanitize (detect Attributes in a file) could be the way to go here in that if you define attributes, you can then overlay the VBE, otherwise, just carry on as a normal import.

@joyfullservice
Copy link
Owner

I wonder if a sort of reverse sanitize (detect Attributes in a file) could be the way to go here in that if you define attributes, you can then overlay the VBE, otherwise, just carry on as a normal import.

My thought exactly! If no hidden attributes are detected, then we can simply use the standard import. Otherwise we will use the alternate VBE import after adding the header section in a temporary file based on the SaveAsText export, which contains the attributes.

@joyfullservice joyfullservice self-assigned this May 26, 2021
@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

The upshot of this might also make the exported files more portable, too.

For instance, if I import the raw exported files from the addin via the IDE > import file dialog, classes will invariably be imported as a plain module. If the class info as shown below is kept with the export (or manually added later), that works perfectly (though...breaks the loadfromtext routine, which sill befuddles me why one will work but not the other, and if you make the other, the first breaks). But if you're using the VCS, and you want to hand a file to someone else that doesn't have it (namely a class/module would be the two most common IMO) that other user doesn't need to know much at all to import it.

This also would allow users who get third party addons modules/classes and import them, they don't need to do / know much other than they plop it in the modules folder and hit build from source, or go the old fashioned way and IDE> File Import, and the two methods will be consistent.

I am curious, though, as the saveastext doesn't seem to consistently export those same attributes, so this might be another hurdle to manage. One step at a time :)

@joyfullservice
Copy link
Owner

joyfullservice commented May 26, 2021

Hmm... Maybe we should always use VBE export for modules... Is there anything that would be in the SaveAsText version that isn't included in the VBE export?

The other nice thing about the native VBE export files is that you can drag them and drop them directly into a project in the IDE and they import automatically. They would definitely be more portable.

@hecon5
Copy link
Contributor Author

hecon5 commented May 26, 2021

A long while back I came across a SO thread going over it, but abandoned the idea when I found the legacy addin.

Upon reread, looks like one poster handles query adjustment, too..........

joyfullservice added a commit that referenced this issue May 26, 2021
This change allows us to utilize hidden VBE attributes in source code to define things like default members and description text. See #230 for additional details.
joyfullservice added a commit that referenced this issue May 26, 2021
@joyfullservice
Copy link
Owner

I have just rolled out this change to the dev branch. Please note that all exported code modules will change the first time you run an export. This is expected since we are now using the VBE.Export format instead of the SaveAsText. Also note that this change only affects standard and class modules. It does not affect Forms and Reports, which will continue to use the combined export file generated from SaveAsText.

@joyfullservice
Copy link
Owner

A side benefit of this change is that it has slightly improved the performance of the code export, especially when large numbers of code modules are involved.

Also note that if you manually import hidden attributes to a code module, and you have Fast Save turned on, you will want to do a full export to make sure those changes get saved to the source files. The change detection only looks for changes in the VBA code. It does not export each object and compare the file hash to pick up on the changes to hidden attributes. I tried that initially, but it was many times slower than just checking the VBA source behind the object. Since most people will manage these changes to hidden attributes using the source files themselves, and build from source to bring them into the project, I don't really think this will be a problem.

@hecon5
Copy link
Contributor Author

hecon5 commented May 27, 2021

I'll check them out momentarily!

@hecon5
Copy link
Contributor Author

hecon5 commented May 27, 2021

Well, like all good ideas I have, this one's got some bumps.

When using a prior version to build the db from source, the header info makes things unhappy. This may be an issue when others try to build. Perhaps we may need to release a new version soon with this feature to avoid that too much?

There have been some other improvements, too, so may be a good time.

@joyfullservice
Copy link
Owner

Yes, this is definitely a "breaking change" in the sense that it will not be compatible with versions that use the SaveAsText methods to export/import modules. You will need to export changes using the new version, and build using the new version. You won't be able to export with one version and build using the other.

One thing that we probably should add is a version check that does not allow a build using files from an incompatible version. With this in mind, perhaps this should be released as version 3.4.0, and only build when the exported version is 3.4.0 or greater. (I am thinking that the merge functionality that I am currently working on will be released as version 4.0.0 since it adds a major functionality change.)

@hecon5
Copy link
Contributor Author

hecon5 commented May 27, 2021

Success! Looks like they export / import fine on the VCS repo.

I had to check out commit d031557 , remove the header from clsDbModule, build from source on that file, then install that. From there, I merged d2e38f4 into my repo (I didn't want to be fighting also with the new forms/other features yet).

Which, brings me to my next request: if you're going to manage the main repo for this (I applaud it, and welcome/ thank you for heading it up, because it means less for me/others to do), if you could do work on other branches, and then merge them into dev when they're done? That way, users/me won't have to wonder what the new table tblStrings and tblTranslation or other features are or if they work.

Anyway, the preservation of attributes works! So, I think we can close this, unless you've got other improvements that need to happen.

@hecon5
Copy link
Contributor Author

hecon5 commented May 27, 2021

With this in mind, perhaps this should be released as version 3.4.0, and only build when the exported version is 3.4.0 or greater

I agree! With the other BOM issues and the like, I think this is a great idea. If 3.4.0 is released, we should remove the translation / conflict forms from the code base and put them in their respective branches, as I think they may distract from the 'working' base, even if most users won't see them.

@joyfullservice
Copy link
Owner

Sounds good. Sorry about the extra clutter with the new features in progress.... Initially I was thinking I would just slowly work them into the main dev branch as I had time an opportunity, but both have become fairly complex, and are probably better off in their own feature branches till they are ready to roll into dev for final testing.

As a side note, I am really enjoying the ability to do this kind of branched development work in Microsoft Access! That "build from source" concept has truly revolutionized the way I do database development! 😄

joyfullservice added a commit that referenced this issue May 27, 2021
These objects are being implemented in specific feature branches which will be rolled into `dev` when they are completed. See discussion in #230
@hecon5
Copy link
Contributor Author

hecon5 commented May 27, 2021

As a side note, I am really enjoying the ability to do this kind of branched development work in Microsoft Access! That "build from source" concept has truly revolutionized the way I do database development! 😄

Yes; with the legacy VCS me and a few others on my team were able to rapidly add in some new features between the two of us that wouldn't have been possible before. With the addin, the skids are greased, and it's even easier!

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 1, 2021

As I have been doing some testing of this; I have been finding some issues with reverting old code; namely pre/post this implementation.

I'm wondering if the performance hit to check which version of the file is being imported would be worth it or not, at least as a non-default option.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 2, 2021

Actually, after more testing, we probably need to revisit this, possibly reopen and implement detection. Worst case, this may need to be combined into milestone 4.0.

I still think the files should export with the new VBE.Export method used now, it's import that has some ragged edges. For me, I expect build times to be longer than export (I'm usually exporting, not building as the primary developer), the performance hit to detect changes is acceptable.

I've found a bunch of cases where if you rollback changes to a class/module in git the import will break things, quite often.

Some thoughts on how to speed up building based on whether loadfromtext or VBE.Import is used:

In order from what I think might lead to the fastest build to the slowest, or most desirable to least:

  1. Put what method was used to export/import in the index. Upsides: fast, it's already loaded, and we're using info from there to build. Downsides: imported modules that are 'new' still need to be detected, and it's ignored by git, so will probably still not work this way.
  2. Scan file to see if it contains Attributes: (SaveAsText does not output Attribute VB_Name = "clsSillyName", so this could be the easiest). Also, this is probably the most reliable, as the other methods might not have an easy mechanism to detect if the file has since been setup to be imported via VBE.Import or not, especially for drag&drop files.
VERSION 1.0 CLASS
BEGIN
  MultiUse = -1  'True
End
Attribute VB_Name = "clsSillyName"
  1. Add option to Addin for import/export method. Upsides: consistent within addin, downsides: drags along old settings.
  2. Add import/build method to .json file that holds PrtMip or other information. Upsides: would be in git. Downsides: might not be correct for new files.

@joyfullservice
Copy link
Owner

@hecon5 - Thanks for taking the time to flesh out these thoughts! I have been thinking over this as well over the past few days, and I think I would like to expand upon No. 2 in your list of options. Currently we are rewriting the UTF-8 file as ANSI (system encoding) just before importing anyway, so this gives us the perfect opportunity to check the first 10 lines for the header information, and add it in, if necessary, when rewriting the file. Since we are already doing the file rewrite, I don't think we should see much of a performance penalty.

I like this approach because going forward, there will always be both ways of exporting a code module. Although this add-in will exclusively use the VBE Export, the source files it interacts with could be in either format. This might be because they were created using an older version of this add-in, or from tool that uses the SaveAsText approach. It seem much more robust to support the import of both formats for the long term, knowing that both are valid types of module source code export files. It will also help those that might be moving over to this tool from another system, since they can continue to use their existing code base.

@joyfullservice joyfullservice reopened this Jun 3, 2021
@hecon5
Copy link
Contributor Author

hecon5 commented Jun 3, 2021

I was thinking, another way (twofer?) would be to use a different extension, which could further accelerate import detection. SO Answer for File Types, as this also seems to be more inline with the IDE's Import, too:
image

If the file is .cls it's going to be a class, so it'd better have the Class "preamble". If it's a .bas file, then it MIGHT be a class, but probably isn't; but we should still do detection on it.

While this may lead to rename headaches, it also makes naming more consistent with the rest of the Office IDE ecosystem, makes detection easier, importing easier, and if you do end up rolling back a change, you're not causing yourself a ton of angst and headaches when your class imports as a standard module.

@joyfullservice
Copy link
Owner

I do like the idea of moving to the *.cls extension for class modules. As you pointed out, this is pretty standard across the VBA (and VB) world. My initial hesitation was due to the complexity of dealing with naming conflicts and purging orphaned files, but I think I have a good solution for that now, so I will plan to move forward with using the *.cls extension. 👍

joyfullservice added a commit that referenced this issue Jun 4, 2021
This is more consistent with the default IDE behavior and file naming conventions. See #230
@joyfullservice joyfullservice added this to the Release 3.4.0 milestone Jun 5, 2021
joyfullservice added a commit that referenced this issue Jun 5, 2021
This allows us to natively import legacy SaveAsText modules and classes, as well as VBE modules and classes. The VBE header is dynamically added if needed. Closes #230
@joyfullservice
Copy link
Owner

Automatic header detection has now been implemented! VBE or legacy SaveAsText files are correctly imported with any missing headers automatically added before import.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 9, 2021

Export / import doesn't seem to be working for me after testing, not sure what happened, because it was working.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 9, 2021

Doing some more testing, but a class exported with the legacy SaveAsText isn't importing as a class for me, and the header isn't being added. I've tried with several different classes, none appear to import as class or be detected as a class.

[Edit]
oooohk...this one's my fault...at some point, I somehow exported a file that stripped out the header. This works now. derp...

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

No branches or pull requests

2 participants