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

refactor(pass-style): Avoid name convention conflict #2666

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Dec 19, 2024

closes: #XXXX
Agoric/agoric-sdk#10751

Description

We generally use names of the form ^([A-Z]\w*)I$, i.e., identifiers beginning with a capital letter and ending with a lone capital "I", for interface guards. However there were some uses of the same form of name for typescript interfaces. Because interfaces and interface guards are adjacent concepts, this could be confusing. This PR fixes this for endo. The companion Agoric/agoric-sdk#10751 fixes it for agoric-sdk. Because the endo names in question were not exported, there is clearly no dependency between these two PRs.

Security Considerations

Consistent naming conventions makes code more reviewable, which is good for security.

Scaling Considerations

none

Documentation Considerations

Consistent naming conventions helps documentation and its readers.

Testing Considerations

none

Compatibility Considerations

Because these names are not exported, and are only type names erased prior to execution, none.

Upgrade Considerations

Because these are only type names that are erased prior to execution, none.

@erights erights force-pushed the markm-avoid-name-convention-conflict branch from 6c64ec1 to 8998ca7 Compare December 20, 2024 00:01
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

@erights
Copy link
Contributor Author

erights commented Dec 20, 2024

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

👌

@erights erights merged commit 59290f6 into master Dec 20, 2024
14 of 15 checks passed
@erights erights deleted the markm-avoid-name-convention-conflict branch December 20, 2024 17:09
mergify bot pushed a commit to Agoric/agoric-sdk that referenced this pull request Dec 20, 2024
)

closes: #XXXX
refs: endojs/endo#2666

## Description

We generally use names of the form `^([A-Z]\w*)I$`, i.e., identifiers beginning with a capital letter and ending with a lone capital "I", for interface guards. However there were some uses of the same form of name for typescript interfaces. Because interfaces and interface guards are adjacent concepts, this could be confusing. This PR fixes this for agoric-sdk. The companion endojs/endo#2666 fixes it for endo. Because the endo names in question were not exported, there is clearly no dependency between these two PRs. 

### Security Considerations

Consistent naming conventions makes code more reviewable, which is good for security.

### Scaling Considerations
none

### Documentation Considerations
Consistent naming conventions helps documentation and its readers.


### Testing Considerations
none

### Compatibility Considerations

Unlike endojs/endo#2666, the type names in question here are exported by their respective modules (and likely packages), so in theory it is possible to break code outside agoric-sdk that depends on these type names. Because these are only type names that are erased prior to execution, this mismatch could only cause static problems, not runtime problems. 

Further, the names in question ***seem*** only intended as helper types for defining other types. Likely, and dependence outside agoric-sdk will only be to those outer types, not to these helper types, in which case an actual compat problem is unlikely. But possible.

### Upgrade Considerations
Because these are only type names that are erased prior to execution, none.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants