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

262 refactor rename nameandversion/librarykey to versionedidentifer 2 #546

Conversation

baseTwo
Copy link
Collaborator

@baseTwo baseTwo commented Sep 17, 2024

Work for

Make the term VersionIdentifier the standard when referring to an ELM library by its name and/or version. Renamed any case of NameAndVersion to VersionedIdentifier to make it all consistent.

Keep the GetVersionedIdentifier() method as internal. I'd like to use the Result pattern for the future instead of passing in a boolean to throw an error or not.

@baseTwo baseTwo requested a review from ewoutkramer September 17, 2024 09:05
@baseTwo baseTwo marked this pull request as ready for review September 17, 2024 09:05
@baseTwo baseTwo enabled auto-merge September 17, 2024 09:06
@richfirely
Copy link
Contributor

richfirely commented Sep 17, 2024

Since your doing this shouldn't we take this opportunity to make this also remove the need for getting parts and building strings? This is off the top of my head so please forgive.
VersionedIdentifier .ToFileName <-- which always has the dash naming convention that we agreed on.
VersionedIdentifier .Name
VersionedIdentifier .Version
VersionedIdentifier .ToString() -- which results in the proper pipe format, but no pipe if version is null.
VersionedIdentifier .ToUri or ToCanonical("https...") which results in the full uri

@baseTwo baseTwo changed the title 262 refactor rename nameandversionlibrarykey to versionedidentifer 2 262 refactor rename nameandversion/librarykey to versionedidentifer 2 Sep 17, 2024
@baseTwo
Copy link
Collaborator Author

baseTwo commented Sep 17, 2024

Since your doing this shouldn't we take this opportunity to make this also remove the need for getting parts and building strings? This is off the top of my head so please forgive. VersionedIdentifier .ToFileName <-- which always has the dash naming convention that we agreed on. VersionedIdentifier .Name VersionedIdentifier .Version VersionedIdentifier .ToString() -- which results in the proper pipe format, but no pipe if version is null. VersionedIdentifier .ToUri or ToCanonical("https...") which results in the full uri

Sounds like a good idea, can we do this as part of another issue?

…l-sdk into 262-refactor---rename-nameandversionlibrarykey-to-versionedidentifer-2
@ewoutkramer
Copy link
Member

My main remark is the same as @richfirely 's - there is actually a VersionedIdentifier type, so I always thought the name "NameAndVersion" meant a string with both components, and we'd use "VersionedIdentifier" for when we talk about the type. So, this is a necessary first stap (I am happy we ditch the unnecessary LibraryKey term), but it's confusing the GetVersionedIdentifier() function does not return a VersionedIdentifier.

@baseTwo baseTwo merged commit ac50bbe into develop-2.0 Sep 18, 2024
2 checks passed
@baseTwo baseTwo deleted the 262-refactor---rename-nameandversionlibrarykey-to-versionedidentifer-2 branch September 18, 2024 11:55
@baseTwo
Copy link
Collaborator Author

baseTwo commented Sep 18, 2024

My main remark is the same as @richfirely 's - there is actually a VersionedIdentifier type, so I always thought the name "NameAndVersion" meant a string with both components, and we'd use "VersionedIdentifier" for when we talk about the type. So, this is a necessary first stap (I am happy we ditch the unnecessary LibraryKey term), but it's confusing the GetVersionedIdentifier() function does not return a VersionedIdentifier.

Will be addressed in

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

Successfully merging this pull request may close these issues.

3 participants