Skip to content
This repository was archived by the owner on Feb 19, 2025. It is now read-only.

Conversation

@r2o3k
Copy link

@r2o3k r2o3k commented Jan 31, 2024

No description provided.

@r2o3k
Copy link
Author

r2o3k commented Feb 1, 2024

@Jyyjy Tagging you for review

@Jyyjy
Copy link
Contributor

Jyyjy commented Feb 1, 2024

I don't think this field is meaningfully different than toolName. Other than requiring the user to specify it. If there is a difference, can we make that clear in the read me (and fix tests)? And if not, should this PR be to make toolname required?

@EandrewJones
Copy link
Contributor

EandrewJones commented Feb 5, 2024

Besides the fact I hate having a field entitled toolName---which screams this is a "descriptive, non-unique value"---be used as a unique id, it can work in theory. If I was a dictator, I'd say we migrate to appId but that would introduce breaking changes.

@r2o3k Can you simply make toolName and toolVersion mandatory fields like you have here with appId and we'll drop appId for now? And we need to fix the tests too.

@EandrewJones
Copy link
Contributor

Closing since this is covered by #411

@EandrewJones EandrewJones deleted the adding-appid branch February 13, 2024 15:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants