Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

chore: Maintain consistency between languages functions#63292

Merged
varungandhi-src merged 7 commits into
mainfrom
vg/consistency
Jun 18, 2024
Merged

chore: Maintain consistency between languages functions#63292
varungandhi-src merged 7 commits into
mainfrom
vg/consistency

Conversation

@varungandhi-src

@varungandhi-src varungandhi-src commented Jun 17, 2024

Copy link
Copy Markdown
Contributor

The different functions in the languages package ought to be mutually
consistent in how they handle language<->extension mappings.

Since we have complex logic in some functions, we need to make
sure that newly added APIs take all the edge cases into account,
and add tests so that we can catch regressions with enry upgrades.

This PR also adds the enry package to depguard, so that people
do not accidentally use enry APIs instead of the corresponding APIs
in the languages package.

Test plan

Added tests.

Changelog

@varungandhi-src varungandhi-src requested a review from mmanela June 17, 2024 14:26
@cla-bot cla-bot Bot added the cla-signed label Jun 17, 2024
@github-actions github-actions Bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels Jun 17, 2024
@varungandhi-src varungandhi-src enabled auto-merge (squash) June 17, 2024 14:27

@mmanela mmanela left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @varungandhi-src . Couple questions/comments added

Comment thread lib/codeintel/languages/extensions.go Outdated
Comment thread lib/codeintel/languages/extensions.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is such a great idea! Will remember this for the future

Comment thread lib/codeintel/languages/extensions.go Outdated
Comment thread lib/codeintel/languages/extensions.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You already clone above, do you need to clone again?

@mmanela mmanela Jun 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also find it surprising that go-enry would return shared state. I am still new to Go and their library ecosystem but I would hope people would return immutable data (or at least data that was a copy )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed this extra clone

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Go natively does not have any concept of immutable data except for a wink and a hand-shake, and IME the ecosystem largely copies the standard library in terms of not trying too hard to avoid footguns.

@varungandhi-src varungandhi-src merged commit 3437f82 into main Jun 18, 2024
@varungandhi-src varungandhi-src deleted the vg/consistency branch June 18, 2024 13:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants