Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Localization #417

Conversation

michaelmairegger
Copy link
Contributor

@michaelmairegger michaelmairegger commented Sep 23, 2024

This PR refactors the code to be able to read the Tabelle from a resource file. I changed the code only for EsigibilitaIVA to show how my idea works.

For that I introduced a new subclass of Tabella, TabellaV2 which contains the new code. Now each key-value-pair Codice-Nome is read from a resource file and allows localization of all values.

If you like this PR and my idea, I can continue to localize other tabellas too.

#416

Tabelle

  • CausalePagamento
  • CondizioniPagamento
  • Divisa
  • EsigibilitaIVA
  • FormatoTrasmissione
  • IdPaese
  • ModalitaPagamento
  • Natura
  • NaturaSemplificata
  • Provincia
  • RegimeFiscale
  • ScontoMaggiorazione
  • SocioUnico
  • SoggettoEmittente
  • StatoLiquidazione
  • TipoCassa
  • TipoCessionePrestazione
  • TipoDocumento
  • TipoDocumentoSemplificata
  • TipoResa
  • TipoRitenuta
  • Remove all V2 unit tests
  • Remove all V2 classes

Validators

  • Localization for validators

@nicolaiarocci
Copy link
Contributor

Hi @michaelmairegger, I'd love to add multi-language support.

I do not think a specialized TabellaV2 is strictly necessary. Please see my alternative pull request (#419) which is essentially doing the same thing, but with no new classes. Try it out, and if it's good on your side you may continue from there and update all the tables. Alternatively, let me know why a TabellaV2 class would be necessary.

@michaelmairegger
Copy link
Contributor Author

michaelmairegger commented Sep 25, 2024

I have added TabellaV2 only to show it for the one class.

The only advantage about my implementation within TabellaV2 is that it reads all the list from resources rather than hardcoding it.

I saw your PR. Initially this was my idea too, to have the keys hardcoded and the value is read from the resources. My thought was that it would be more convinient to have a resource file with the key/values rather than having it to code. The advantage would be, that if e.g. a new Natura will be added that information has to be added only to the Natura.resx file. Othwerwise, we have to add it as translation resource into Natura.resx and to Natura.cs as mapping.

Therefore my solution is only for better maintainability reasons.

@nicolaiarocci
Copy link
Contributor

Makes sense, althought these tables are unlikely to be updated frequently (but we've seen it happening a couple times in the past). I'd say, go on with your approach then, as it only brings advantages over mine.

@michaelmairegger michaelmairegger force-pushed the feature/resource-based-tabelle branch from 781a192 to 79b322e Compare September 25, 2024 15:23
@michaelmairegger
Copy link
Contributor Author

@nicolaiarocci Ok, I will continue. I will add a new class for each Tabella with V2 prefix. This is only temporary since I will add unit tests to ensure I do not forget any key or have a typo in values

@michaelmairegger
Copy link
Contributor Author

@nicolaiarocci I think I have done everything. If this fits for you I can remove all Legacy classes and rename all V2 classes.

@nicolaiarocci
Copy link
Contributor

LGTM. I was wondering if, at this point, it would make sense to also add multi-language support to Validatotrs. There aren't many hard-coded strings there, but they could use translation support. As a foreign adopter, what is your opinion?

@michaelmairegger
Copy link
Contributor Author

Ok, I will then continue with refactoring and replace the hard-coded tabellas with the new one.

Yes that would be a nice feature. I can provide a new PR too

@michaelmairegger michaelmairegger changed the title feat: Read tabella from resources to allow localization of the lists feat: Localization Sep 30, 2024
@michaelmairegger michaelmairegger force-pushed the feature/resource-based-tabelle branch from 6ce4d9c to 7937c90 Compare September 30, 2024 06:51
@michaelmairegger
Copy link
Contributor Author

Translation done. If something missing please let me know

@nicolaiarocci nicolaiarocci merged commit 7937c90 into FatturaElettronica:master Sep 30, 2024
3 checks passed
@nicolaiarocci
Copy link
Contributor

Excellent, thank you.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants