« first day (2336 days earlier)      last day (844 days later) » 

Duga
Duga
12:01 AM
RELOAD!
[rubberduck-vba/Rubberduck] 1 opened issue. 3 issue comments.
[Minesweeper] Games Played: 96, Bombs Used: 64, Moves Performed: 13597, New Users: 6
> @BZngr This issue does not apply to public user defined types, since they are not legal in document modules, or any kind of class module.
> Right now it creates private fields such as `something_1` and a public property such as `Something` which is okay - not terrible. But it sure would be nice if the Preview window was writable so it could be tweaked before posting.

Also, most of the time I select the "Create private this type" to hide the private fields so in that case, renaming the fields is redundant anyway. Any chance this option could just leave the field names alone?
 
BZngr
BZngr
Hmm...Yep (after testing)...your are correct as usual. :) Given that, it seems unfortunate that public enums are allowed.
 
Duga
Duga
12:16 AM
 
Duga
Duga
12:40 AM
> Starting with `Public test As Long` and Encapsulating using a Private Type...the results I get are:
```
Private Type TModule1
Test As Long
End Type

Private this As TModule1

Public Property Get Test() As Long
Test = this.Test
End Property

Public Property Let Test(ByVal RHS As Long)
this.Test = RHS
End Property
```

Can you share more about your use case? Based on the above, a `ByVal` parameter mechanism is provided in the Let method and the field identifier is l
 
Duga
Duga
12:51 AM
> **Rubberduck version information**
The info below can be copy-paste-completed from the first lines of Rubberduck's log or the About box:

Rubberduck version [2.5.1.5622]
Operating System: [Win10]
Host Product: [vb6]
Host Version: [sp6]
Host Executable: [N/A]


**Description**
`Variable not referred to` false positive in for loop

**To Reproduce**
```
Public Function CreateNonce(Optional ByVal NonceLength As Long = 32) As String
Dim web_Str As String
> Sure, here's a **before**

`'@Folder("Web.Authenticators")
''
' Digest Authenticator v3.0.8
' (c) Tim Hall - https://github.com/VBA-tools/VBA-Web
'
' Custom IWebAuthenticator for Digest Authentication
' http://en.wikipedia.org/wiki/Digest_access_authentication
'
' @class DigestAuthenticator
' @implements IWebAuthenticator v4.*
' @author tim.hall.engr@gmail.com
' @license MIT (http://www.opensource.org/licenses/mit-license.php)
'' ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Yeah, one sec...
> **Before:**
`' --------------------------------------------- '
' Properties
' --------------------------------------------- '

Public Username As String
Public Password As String

Public Realm As String
Public ServerNonce As String
Public RequestCount As Long
Public Opaque As String

Public Property Get ClientNonce() As String
If auth_pClientNonce = vbNullString Then
auth_pClientNonce = WebHelpers.CreateNonce
End If
ClientNonce = auth_pClientNonce
End Prop
> **Before:**
`

Public Username As String
Public Password As String

Public Realm As String
Public ServerNonce As String
Public RequestCount As Long
Public Opaque As String

Public Property Get ClientNonce() As String
If auth_pClientNonce = vbNullString Then
auth_pClientNonce = WebHelpers.CreateNonce
End If
ClientNonce = auth_pClientNonce
End Property
Public Property Let ClientNonce(Value As String)
auth_pClientNonce = Value
End Property

Public Prop
> **Before:**
`

```
Public Username As String
Public Password As String

Public Realm As String
Public ServerNonce As String
Public RequestCount As Long
Public Opaque As String

Public Property Get ClientNonce() As String
If auth_pClientNonce = vbNullString Then
auth_pClientNonce = WebHelpers.CreateNonce
End If
ClientNonce = auth_pClientNonce
End Property
Public Property Let ClientNonce(Value As String)
auth_pClientNonce = Value
End Property

Public
> **Before:**
`

```
Public Username As String
Public Password As String

Public Realm As String
Public ServerNonce As String
Public RequestCount As Long
Public Opaque As String

Public Property Get ClientNonce() As String
If auth_pClientNonce = vbNullString Then
auth_pClientNonce = WebHelpers.CreateNonce
End If
ClientNonce = auth_pClientNonce
End Property
Public Property Let ClientNonce(Value As String)
auth_pClientNonce = Value
End Property

Public
 
Duga
Duga
1:28 AM
> This is the third time I tried to paste this correctly and I noticed something, RD isn't consistent when it renames the private variables. Each time I ran through the process it gave different results. Below is the most recent attempt. Some names it did okay, others it added _1. I also noticed that RD has now gone back to adding the ByRef again....

```
Public Username As String
Public Password As String

Public Realm As String
Public ServerNonce As String
Public RequestCount As Lo
 
Duga
Duga
1:38 AM
> More info:

When I try to override the field name it complains of a name conflict but there isn't any

![image](https://user-images.githubusercontent.com/11149818/97380136-a1d7f800-1883-11eb-9142-63c418fd91b8.png)
> More info:

When I try to override the property name it complains of a name conflict but there isn't any

![image](https://user-images.githubusercontent.com/11149818/97380136-a1d7f800-1883-11eb-9142-63c418fd91b8.png)
> And with fields of type Variant, it creates both a Let and a Set but the Get is really a mess:

```
Public Property Get Scopes() As Variant
Set Scopes = this.ScopesIf IsObject(this.Scopes) ThenElseIf IsObject(this.Scopes) ThenScopes = this.ScopesIf IsObject(this.Scopes) ThenEnd If
End Property

Public Property Let Scopes(ByVal RHS As Variant)
this.Scopes = RHS
End Property

Public Property Set Scopes(ByVal RHS As Variant)
Set this.Scopes = RHS
End Property
```
> The name collision from earlier could be because I'm in VB6 working on private classes that have public fields. RD might not be aware that each of the classes have their instancing set to 1-Private...
> Hmmm...I do not see a single `ByRef` Let statement in the provided code.

So, starting with your **Before** scenario
```
Public Username As String
Public Password As String

Public Realm As String
Public ServerNonce As String
Public RequestCount As Long
Public Opaque As String

Public Property Get ClientNonce() As String
If auth_pClientNonce = "" Then
auth_pClientNonce = WebHelpers.CreateNonce
End If
ClientNonce = auth_pClientNonce
End Property
Public Pro
> The Get statements for the Variant type bears looking into. It definitely looks like a bug.
> Sorry, I meant ByVal not ByRef. It's hit or miss if it includes them....
> Regarding the comments about a conflict...looks like a different module. I would need to see the what's in the module to comment more.

Regarding the `ByVal`...I'm not seeing the 'misses' - can you point one out?
> I think the Username_1 and Password_1 are are because there are public fields in other private modules in this project. Could it be that RD is unaware of Instancing set to 1-Private?
> It's a different class module for sure
> > Regarding the comments about a conflict...looks like a different module. I would need to see the what's in the module to comment more.
>
> Regarding the `ByVal`...I'm not seeing the 'misses' - can you point one out?

I'll catch one - I'm doing a lot of refactoring...
> Public fields in other private modules may be the cause of the identifer modification...another issue to look into.
> Here's a link to the original code. It's meant for VBA and I'm porting it to VB while cleaning it up. Maybe you can grab it and then you'll have the whole picture.
[Github Repo](https://github.com/VBA-tools/VBA-Web)
> But AFAIK, VBA doesn't know anything about Instancing so go figure...
> Thanks for the link. Quickly looked for the code with the conflicts...but did not spot it. Can you tell me the module name?
> Sure, in the Authenticators folder there are a hand full of files that are all very similar (lots of copy/paste) The code I've been showing you is in the DigestAuthenticator module
 
 
2 hours later…
Duga
Duga
3:54 AM
 
 
7 hours later…
Duga
Duga
11:07 AM
> Finished cleaning up the lib and never saw the ByVal or ByRef thing occur again - must have been brain fog.
 
Duga
Duga
11:24 AM
> A comment on the `Value` vs `RHS` parameter name for the `Let`/`Set` properties:

This was just changed _from_ `value` to `RHS`. I agree `RHS` is pretty ugly, however, I championed the change because the `value` parameter that _was_ being introduced interfered with VBA's ever so "helpful" recapitalization of _every_ instance of the keyword `value` throughout the code.

This caused display issues where:
`Workbook{"A"}.Worksheet("B").Range("A1").Value = 10`
would be displayed
`Workbo
> A comment on the `Value` vs `RHS` parameter name for the `Let`/`Set` properties:

This was just changed _from_ `value` to `RHS`. I agree `RHS` is pretty ugly, however, I championed the change because the `value` parameter that _was_ being introduced interfered with VBA's ever so "helpful" recapitalization of _every_ instance of the keyword `value` throughout the code.

This caused display issues where:
`Workbook("A").Worksheet("B").Range("A1").Value = 10`
would be displayed
`Workbo
> As I said, I'm not morally opposed to RHS except it triggers the Rename Variable alert in the code issues window. Modifying that trigger would suffice as far as I'm concerned...
 
 
2 hours later…
Duga
Duga
1:37 PM
> I think it would indeed make sense to have `RHS` in the allow-list (you'll find it in the inspection settings; click the cogwheel icon in the *inspection results* toolbar, or find the *inspection settings* tab in the RD settings dialog), since it's an identifier that the VBE will automatically use in some instances when implementing an interface (which was part of the rationale for using that identifier name).

I think the default out-of-the-box settings don't include any identifier names th
 
Mathieu Guindon
Mathieu Guindon
Making the preview box editable is out of the question lol
 
BigBen
BigBen
I don't normally use userforms but now am needing to. Potentially dumb question. When populating a userform with data, in my case a combobox with data from a ListObject - should the calling code that creates a New instance do the populating so as to avoid the "smart UI" anti-pattern?
 
FreeMan
FreeMan
@Duga Dude... I've been working so hard to get mySelf to use camelCase and NowYou're TellingMe to use PascalCase? sobs
 
this
this
@FreeMan You could just have both....
 
FreeMan
FreeMan
I do have an ugly mix of both at this point. And I get all sorts of git issues when I forget what I'm doing and accidentally Dim myValue in one module where I have Dim MyValue in some other module and get 10000 diffs...
 
this
this
1:52 PM
#TheJoyOfCasePreservingLanguage
 
BZngr
BZngr
2:15 PM
On a related note, it seems to me that we could/should have a Casing validation check added to the RenameRefactoring. If you pick a name that is valid and free of conflicts - the user gets a green light. But if the rename is different casing than another existing non-conflicting declaration (e.g., a parameter) the user gets more changes than bargained for.
2
 
this
this
Agreed. It should just warn the user, though in case it's deliberate
 
Mathieu Guindon
Mathieu Guindon
would be slick if we could grab the actual names list the VBE is using
 
this
this
There will be situations where the user will really want Value but due to some crappy code somewhere value will be forced. That is especially the case with a control named value, since a control has higher precedence (for a lack of better term) than a VBA variable.
 
Mathieu Guindon
Mathieu Guindon
> This operation looks like it will affect the character casing of unrelated and otherwise unaffected code.
kinda
 
BZngr
BZngr
Probably could provide a count of unrelated declarations - so the user gets some sense of how many references will change.
It might give them pause for some cases.
 
Mathieu Guindon
Mathieu Guindon
2:23 PM
hm. Isn't this entire discussion a non-issue if the rename refactoring popped a preview box listing all the changes (and letting the user uncheck changes they don't want to make)?
 
BZngr
BZngr
Well...that's how I found it...In EF's preview box, the casing does not propagate. So it looks good in the Preview.
 
Mathieu Guindon
Mathieu Guindon
hm
 
BZngr
BZngr
Maybe that's just a short coming of the preview method used for EF.
 
Mathieu Guindon
Mathieu Guindon
it can't hurt to have the validation label /warning about possible unintended casing changes anyway
 
this
this
@MathieuGuindon it would need to include all occurrences, not just the code being refactored.
 
BZngr
BZngr
2:27 PM
Add a 'view collateral damage' button.
3
 
Mathieu Guindon
Mathieu Guindon
@this yeah... It changes the refactoring logic a bit, ...would allow for an option to check in comments and string literals too
(for some values of "a bit")
 
Duga
Duga
2:51 PM
> There are two issues identified above:
1.` Variant` property code generation has a bug (a regression bug). I have a PR nearly ready for this.
2. The `EncapsulateFieldRefactoring` conflictfinder does not properly account for MemberAccessExpressions using `Me` in local procedures. The conflicts detected above are a conflict between the a Property name and a parameter where an assignment like `Me.ClientId= ClientId` occurs. A PR for this is in the works.
> This PR fixes a regression bug identified in #5609 resulting from PR #5578.
 
Duga
Duga
3:17 PM
[rubberduck-vba/Rubberduck] build for commit 5c4d214d on unknown branch: AppVeyor build succeeded
 
Duga
Duga
3:28 PM
[rubberduck-vba/Rubberduck] BZngr pushed commit 5c4d214d to next: Fix regression bug
Merge pull request #5611 from BZngr/EF_VariantFormat

Fix EncapsulateFieldRefactoring regression bug
 
this
this
since it's a small bug fix PR, figured it's OK to insta-merge it, considering there's more test cases than code. :)
 
BZngr
BZngr
:)
 
Duga
Duga
3:50 PM
[rubberduck-vba/Rubberduck] build for commit d53e06f4 on next: AppVeyor build succeeded
 
this
this
So that leaves us with #5586, #5591, #5602 as PRs that are older than a week. I can't merge the first 2 since they are mine. I have no further work for those PR unless someone want to put in a last minute comment. I'm good to merge @IvenBach's 5602 if he has no further work and nobody else has comments.
 
Duga
Duga
> This PR fixes a bug identified in #5609. The `EncapsulateFieldConflictFinder` did not account for the use of a `Me` qualifier for scenarios like:

```
Public ClientID As Long

'If the 'Me' qualifier was not present, the parameter would identified as a conflict because it changes the assignment logic.
Public Sub Init(ClientID As Long)
Me.ClientID = ClientID
End Sub
```
 
Duga
Duga
4:28 PM
[rubberduck-vba/Rubberduck] build for commit 90946e70 on unknown branch: AppVeyor build succeeded
 
IvenBach
IvenBach
4:41 PM
@this it could/should have a mock test for proving that ignored tests don’t display when the Toggle is off. I didn’t check if that was possible with the current api on test explorer.
If that test isn’t needed then it’s good to go.
 
this
this
I do not think we have any UT that involves the viewmodel.
But need to double check that assertion. If there's nothing in the RubberduckTests testing the viewmodel, then that's OK, I guess.
 
Duga
Duga
5:10 PM
 
IvenBach
IvenBach
5:38 PM
When I was working on my PR I checked but didn’t see any tests for test explorer, hence no tests.
 
this
this
a tester that's untested. Where's that commitstrip about testing the unit testing framework...
 
IvenBach
IvenBach
We Arrive home to our ash pile. No structures lost and it was a block away.
Good jeorb there firepeeoples! Keeping everyone safe.
 
this
this
wait, so you left an ash pile and came back and it was still an ash pile?
 
Duga
Duga
> **Justification**
`Refactorings` that create new elements or rename existing elements, check that the declaration's proposed identifier:
1. Is a valid VBA identifier
2. Does not result in a name collision with another declaration

There is a third evaluation that would be helpful: Casing changes.

If a new or renamed declaration identifier is conflict-free but matches another declaration's identifier differing only with respect to casing, then the new declaration's identifier casing
 
IvenBach
IvenBach
Yeah. Glad we didn’t loose anything. Could have been much worse.
Hope all you ducks have a 72 hour kit available and ready for hasty evacuations.
Don’t forget legal documents also. Those are a pain to replace.
 
this
this
5:52 PM
@IvenBach Better loosen that noose. ;-)
 
IvenBach
IvenBach
Neverending battle with #Words...
 
BigBen
BigBen
#EnglishIsHard
 
IvenBach
IvenBach
My failing 4 years of it in HS proves it!
 
this
this
Homonyms be the work of debbil, I tell ya!
 
IvenBach
IvenBach
6:10 PM
I prolly should have been diagnosed with dyslexia as a kid. So glad I got numbers right.
 
 
2 hours later…
Duga
Duga
8:24 PM
> Close #4541. Opening as WIP to enable review of current work. Want to make sure I'm taking this in the right direction before going any farther.
[rubberduck-vba/Rubberduck] build for commit f8f5d869 on unknown branch: AppVeyor was unable to build non-mergeable pull request
BUILD FAILURE!
 

« first day (2336 days earlier)      last day (844 days later) »