-
Notifications
You must be signed in to change notification settings - Fork 43
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
Simplify server errors and drop some unused code #1095
Conversation
635ebcb
to
80ac668
Compare
agama-server
errors@@ -119,14 +104,14 @@ async fn keymaps(State(state): State<LocaleState>) -> Json<Vec<Keymap>> { | |||
async fn set_config( | |||
State(state): State<LocaleState>, | |||
Json(value): Json<LocaleConfig>, | |||
) -> Result<Json<()>, LocaleError> { | |||
) -> Result<Json<()>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generic comment for whole this PR.
- I like reduction of anyhow errors
- I do not like this change in method signature. I think it is better to return specific error and use upper
Error
only when it can return more types of error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of using the same error is that you only need to implement the IntoResponse
trait once. Initially, I called it ApiError
, but in the end, I decided to use a single type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, if you prefer, I could keep the LocaleError
and implement IntoResponse
only for that kind. It looks good to me too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you cannot have IntoResponse for whole Enum? If it is implemented for Error, I kind of expect it can process also element of Enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe some kind of trait can help there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is implemented for the whole enum (the crate::error::Error) and that's the reason I am returning Error
instead of LocaleError
. After all, LocaleError
knows nothing about Error
.
Adding any trait is overcomplicating things, in my opinion. So I would go for:
- keep it as it is (returning
Error
) or... - returning
LocaleError
and implementingIntoResponse
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, reason why I do not like it is because now it really see only use case for respond, but if it will be used in other parts of rust, then user of this function needs to handle any generic error, while we know that it can product only LocaleError.
So how I do see it is, that if we want to keep extensibility and allow in future to other types of Error in this function, lets change it to Error. But if we know that we always produce only LocaleError ( which I kind of expect ), then I prefer to return minimal abstraction it uses.
But ( and it is big BUT ) I do not care much in web functions that is short and really serve only as connection to real code and do some error translation. But this specific function is quite big and I would say many of its part is not web specific. So maybe if it lives elsewhere and this web function just call other method and translate its errortype into Error. Then it would much nicer.
And to conclude it, I still see this PR as improvement, so feel free to merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I fully agree, this code does not belong here but it is a step in the right direction.
I have added a note to this Trello card so it can be addressed later.
let mut data = state.locale.write().unwrap(); | ||
let mut changes = LocaleConfig::default(); | ||
|
||
if let Some(locales) = &value.locales { | ||
for loc in locales { | ||
if !data.locales_db.exists(loc.as_str()) { | ||
return Err(LocaleError::UnknownLocale(loc.to_string())); | ||
return Err(LocaleError::UnknownLocale(loc.to_string()))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it needs ?
here? it looks kind to strange to return Err and using ?
for returning it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it returns a LocaleError
, not a Error
itself. In the next iteration, my idea is to put most of this code in a separate struct, and keep this function to just do the arguments lifting. At that point, we could get rid of this return Err
and the code will look better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as written in #1095 (comment) feel free to merge it
After a few months of work, it is time to merge the `architecture_2024` branch into `master`. It is still a work-in-progress, but all the efforts should be go now against that branch. ## Pull requests * #1061 * #1064 * #1073 * #1074 * #1080 * #1089 * #1091 * #1092 * #1094 * #1095 * #1099 * #1100 * #1102 * #1103 * #1112 * #1114 * #1116 * #1117 * #1119 * #1120 * #1123 * #1126 * #1129 * #1130 * #1131 * #1132 * #1133 * #1134 * #1136 * #1139 * #1140 * #1143 * #1146 ## Other commits * 8efa41f * 9e2dec0
Prepare for releasing Agama 8. It includes the following pull requests: * #884 * #886 * #914 * #918 * #956 * #957 * #958 * #959 * #960 * #961 * #962 * #963 * #964 * #965 * #966 * #969 * #970 * #976 * #977 * #978 * #979 * #980 * #981 * #983 * #984 * #985 * #986 * #988 * #991 * #992 * #995 * #996 * #997 * #999 * #1003 * #1004 * #1006 * #1007 * #1008 * #1009 * #1010 * #1011 * #1012 * #1014 * #1015 * #1016 * #1017 * #1020 * #1022 * #1023 * #1024 * #1025 * #1027 * #1028 * #1029 * #1030 * #1031 * #1032 * #1033 * #1034 * #1035 * #1036 * #1038 * #1039 * #1041 * #1042 * #1043 * #1045 * #1046 * #1047 * #1048 * #1052 * #1054 * #1056 * #1057 * #1060 * #1061 * #1062 * #1063 * #1064 * #1066 * #1067 * #1068 * #1069 * #1071 * #1072 * #1073 * #1074 * #1075 * #1079 * #1080 * #1081 * #1082 * #1085 * #1086 * #1087 * #1088 * #1089 * #1090 * #1091 * #1092 * #1093 * #1094 * #1095 * #1096 * #1097 * #1098 * #1099 * #1100 * #1102 * #1103 * #1104 * #1105 * #1106 * #1109 * #1110 * #1111 * #1112 * #1114 * #1116 * #1117 * #1118 * #1119 * #1120 * #1121 * #1122 * #1123 * #1125 * #1126 * #1127 * #1128 * #1129 * #1130 * #1131 * #1132 * #1133 * #1134 * #1135 * #1136 * #1138 * #1139 * #1140 * #1141 * #1142 * #1143 * #1144 * #1145 * #1146 * #1147 * #1148 * #1149 * #1151 * #1152 * #1153 * #1154 * #1155 * #1156 * #1157 * #1158 * #1160 * #1161 * #1162 * #1163 * #1164 * #1165 * #1166 * #1167 * #1168 * #1169 * #1170 * #1171 * #1172 * #1173 * #1174 * #1175 * #1177 * #1178 * #1180 * #1181 * #1182 * #1183 * #1184 * #1185 * #1187 * #1188 * #1189 * #1190 * #1191 * #1192 * #1193 * #1194 * #1195 * #1196 * #1198 * #1199 * #1200 * #1201 * #1203 * #1204 * #1205 * #1206 * #1207 * #1208 * #1209 * #1210 * #1211 * #1212 * #1213 * #1214 * #1215 * #1216 * #1217 * #1219 * #1220 * #1221 * #1222 * #1223 * #1224 * #1225 * #1226 * #1227 * #1229
agama-server
crate, unifying many.Locale
D-Bus API that are irrelevant for inter-process communication.IntoResponse
implementations.