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

OH2-182 Standardize error message #262

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

ArnaudFonzam
Copy link
Contributor

@ArnaudFonzam ArnaudFonzam commented Apr 25, 2023

See OH2-182.

vaccine.isnotcreated = Vaccine is not created.
vaccine.isnotupdated = Vaccine is not updated.
vaccine.isnotdeleted = vaccine is not deleted.
vaccinetype.isnotcreated = vaccine type is not created.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't all message text start with a capital letter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update its. tky

@@ -0,0 +1,16 @@
package org.isf.shared;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing license header

@@ -0,0 +1,16 @@
package org.isf.shared;

public class FormatErrorMessage {
Copy link
Collaborator

@dbmalkovsky dbmalkovsky Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this even necessary? You are just working against the standards that exist for the project for a long time
and implementing new "standards" . This just seems wrong and unnecessary.

And putting this is a package called "shared" seems ironic at best as it appears to be only for the web based client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please where it should be. it is necessary because the error that comme from the core with format "angal.package.errormessage.msg" and the front-end just need "package.errormessage" it doesn't needs angal and msg which are on those errors message.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is (and maybe most people would disagree with me) that it seems confusing that two different "standards" are used in the same project. Why did the front-end feel like they needed a different "standard format"? There are lots of "standards" in the project that I'd like to change but don't because it would make things difficult for anyone who works on multiple parts of the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay i think it is better to discuss with @mwithi to have the good way to do because the work that i made was to analyse and give a solution of the problem.

Copy link
Member

@mwithi mwithi May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options are 2:

  1. api responses with a code and a key, and the key is translated on the "ui" side (or any other "api-consumer") -> so you don't need bundles with translations here

or

  1. api responses with a translated error -> we need bundles here, so we need a "standard" way all over the project

Let's go back to the analysis (under OH2-182) to better clarify what are we going to achieve here (and clean also a lot of dead code as I've already pointed out in the comments).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

@mwithi mwithi marked this pull request as draft July 5, 2023 23:09
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