Getting profile or map should not require scope

superfaceai / service-client

Library provides client for superface backend apis.

MIT License

5 stars 1 forks source link

Getting profile or map should not require scope #69

Closed janhalama closed 2 years ago

janhalama commented 2 years ago

Description

This PR fixes getProfile, getProfileAST, getProfileSource, getMap, getMapAST, getMapSource functions. Parameters scope and version are no longer required to get profile or map resources.

BREAKING CHANGE Parameters of getProfile* and getMap* functions has been changed.

Motivation and Context

Fixes: https://github.com/superfaceai/service-client/issues/67

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTION_GUIDE document.
[x] I haven't repeated the code. (DRY)
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

janhalama commented 2 years ago

@kysely I checked Air and it does work well with profiles without scope.

janhalama commented 2 years ago

why keep the profile id components separate? Why not simply expect a valid profileId

@Jakub-Vacek can you confirm that CLI works with profile id components and not full profileId.

janhalama commented 2 years ago

why keep the version required? A request for the latest without specifying a version seems valid to me.

Yes that makes sense to me too. @Jakub-Vacek what do you think?

Jakub-Vacek commented 2 years ago

Currently, CLI http functions (profile and map related) calls directly ServiceClient.fetch where it passes profileId as a string. On the other places we use profileId as ProfileId and optional (which is imo correct - user does not have to pass version expecting that he gets the latest profile/map) version.

CLI will be able to work with both approaches with very little effort. I think the question is more about what is the responsibility of service client. When we decide to pass profile components it will be responsible for construction of the URL when we will pass valid profile ID this responsibility is on CLI/AIR.

kysely commented 2 years ago

The other question is whether ServiceClient should expect its users to understand the profile/map IDs components. But since this is aimed for use in CLI/front end, I think it's reasonable.

In that case I'd suggest keeping the components separate but still make the version optional.

janhalama commented 2 years ago

@Jakub-Vacek @kysely I made version optional parameter. 🙏 review once again.

janhalama commented 2 years ago

Oh I missed that version is required in case of map. Thank you!

janhalama commented 2 years ago

putting all of the document ID components into an object seems like an easier API for me

I wanted to pass optional parameters as options, but as all of the parameters are actually document ID components your suggestion makes sense.