opral / inlang-sherlock

Issue tracker for Sherlock
https://marketplace.visualstudio.com/items?itemName=inlang.vs-code-extension
2 stars 0 forks source link

ship default human ids in sherlock already #75

Closed samuelstroschein closed 3 months ago

samuelstroschein commented 4 months ago

Context

We can avoid discussions like https://discord.com/channels/897438559458430986/1247937929876082839/1248258473851093054 and substantially improve the UX of extracting messages already by shipping human ids in sherlock on extraction before importers/exporters arrive.

Proposal

Ship default filled in human ids in sherlock now to:

CleanShot 2024-06-06 at 08.58.16@2x.png

Scenarios

BEST CASE:

The human ids sherlock uses will be the human ids that the SDK uses which means no aliases will be used once importer/exporter ships

WORST CASE:

The human ids differ from the importer/exporter ids the SDK will use. In that case, the human ids will be treated as aliases. AKA Status quo. Still a better UX for sherlock and paraglide users in the meantime.

samuelstroschein commented 4 months ago

@martin.lysk1 @jldec is a human id generator function exposed by the sdk?

const id = generateMessageBundleId()  
martin-lysk commented 4 months ago

Yes this should do the trick:

https://github.com/opral/monorepo/blob/fb731c9c25958a8df089c636217fbb7916bdddbd/inlang/source-code/sdk/src/storage/human-id/human-readable-id.ts#L5

felixhaeberle commented 4 months ago

I will enable this for the experimental usage of human readable ids.

samuelstroschein commented 4 months ago

I will enable this for the experimental usage of human readable ids.

the proposal is explicitly to enable this now for everyone. we don't have to wait for sdk features.

were you confused by the proposal or have concerns about enabling this now for everyone?

martin-lysk commented 4 months ago

We talked about this earlier and I just recaled you rejected this back than for the reason you outlined in the worse case. If we introduce human identifier we will end up with two human ids for one message bundle because we would import those messages and the id's we find will be handled as foreign id's inside of aliases and we would create an identifier for each message on creation.

We could check if the id is a human id (each word is known) already and don't create an alias?!

samuelstroschein commented 4 months ago

We could check if the id is a human id (each word is known) already and don't create an alias?!

Yes, that's what I had in mind with this proposal.

(Back when we discussed introducing human ids 8-12 weeks ago, I was not aware how long it would take. So, I'd say now ship it in sherlock and be aware in MESDK-66 that messages that obey to a human readable id should not be imported as alias)

felixhaeberle commented 3 months ago

were you confused by the proposal or have concerns about enabling this now for everyone?

This would hurt the vast majority of users (like everybody right now) who have other naming systems. Providing them with this wrong default (in their case) would dramatically reduce DX & lead to inconsistent key naming which they gonna try to avoid.

"Of course, they don't have to accept this default" you might say, but this really feels like the wrong thing to do from Sherlock staying unopinionated about keystyles.

Results: Lots of open issues & bugs where people request other defaults or options.

Worst: User switches to other solution which is unopinionated about keystyle.



Proposed solution:

samuelstroschein commented 3 months ago

This would hurt the vast majority of users (like everybody right now) who have other naming systems.

I heavily doubt this. Can you ship this feature and if users complain deactivate it again?

We are running into endless discussions with naming things. Just ship this.

jldec commented 3 months ago

fyi randomHumanId() is now exported on the @inlang/sdk api.
https://github.com/opral/monorepo/pull/2930

samuelstroschein commented 3 months ago

@felix.haeberle are you gonna ship this?

felixhaeberle commented 3 months ago

yes, its on my list for today.

felixhaeberle commented 3 months ago

@samuel.stroschein Satisfied?

Bildschirmfoto 2024-06-17 um 14.25.41.png



I can't change the Enter/Escape part.

I haven't done any opt-in / opt-out behavior as of now. Will do that with the first complain (100% sure about people will complain and want this to be modified) – thought about sherlock.extract.autoHumanId which defaults to true but can be set to false.

samuelstroschein commented 3 months ago

I haven't done any opt-in / opt-out behavior as of now

I don't think it will be needed.

samuelstroschein commented 3 months ago

yes, baba let's goooo!

jldec commented 3 months ago

👶 🚀