-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add a recursive mail folder scan option to ScanMailboxTask #1260
Conversation
Ah, too bad! I think we can configure circe to ignore non existing fields when mapped to an optional, but I have to go read first. Another way is creating a migration task - I just did this for another case, code is in Edit: I tried it first in the repl and there it is working, strange 🤔 . See this example: import $ivy.`io.circe::circe-core:0.14.1`
import $ivy.`io.circe::circe-generic:0.14.1`
import $ivy.`io.circe::circe-parser:0.14.1`
import io.circe._
import io.circe.parser
import io.circe.generic.semiauto._
case class Person(name: String, flag: Option[Boolean])
implicit val dec: Decoder[Person] = deriveDecoder
val pStr = """{"name":"Robert"}"""
val p = parser.decode[Person](pStr)
// Right(Person(Robert, None)) Just to reassure: it didn't work with old data when specifying |
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.
thank you so much, looks great! 👍🏼
for { | ||
res <- result | ||
search <- searchMails(a)(folder) | ||
} yield SearchResult(res.mails ++ search.mails, res.count + search.count) |
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.
nit picking, only fyi: maybe this can be simplified using traverse
(from cats).
for {
subfolders <- a.listFolders(Some(f))
res <- subfolders.traverse(searchMails(a))
} yield res.foldMonoid
but requires to extend the SearchResult
a bit (moving the combining stuff into a monoid instance)
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.
I added the combining stuff here: eikek/emil#322. I hope I used the correct things here, since you wrote res.foldMonoid
here and I only found the .combineAll
function from cats.Monoid
.
Regarding the dependency: if you're done, I can merge the emil PR and do a release. |
4729ece
to
cb8b74f
Compare
Hey! Sorry for the long delay. I tried using an |
No worries at all! Thank you very much for the well done contribution!
Don't worry - it should work with Option. I will test it against my database before a release. I'll push a release for emil soon than this PR can get updated with the new version and should then pass CI. |
subFolders <- a.listFoldersRecursive(Some(folder)) | ||
foldersToSearch = Vector(folder) ++ subFolders | ||
search <- foldersToSearch.traverse(searchMails(a)) | ||
} yield searchResultMonoid.combineAll(search) |
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.
This is totally fine as it is. just fyi: you can have the compiler to "look up" a monoid instance for some type (because how it is implemented in cats) with this: Monoid[SearchResult]
that would do just the same, scala looks by default in the companion object for stuff. As an alternative, cats provides some syntax enhancements that you get by importing cats.implicits._
(this gets all, you can also import à la carte). With this import you should be able to do search.foldMonoid
.
There is a new release of emil now available - this change should work once rebased on master. |
cb8b74f
to
47e7ad7
Compare
Hey @eikek we rebased the PR, but the CI seems to be hanging... |
I have to approve CI for new committers. 🤷🏼 Thanks! There is only one strange thing: in the meantime we have French translation (!) and from the diff on this PR it seems to be missing in the |
47e7ad7
to
cdbc73b
Compare
Ah yes, that was because we rebased 5 days ago and thought the CI was working so long. |
Ah thanks! Sorry, for some reason i missed the rebase 5 days ago - probably missed the notification. I'm boldly pinging @jgirardet perhaps you have again time to take a look at the French translation in this PR? (please don't feel pressured! just asking) We can also fix them later ofc! |
@@ -311,6 +317,8 @@ fr tz = | |||
, documentLanguageInfo = | |||
"Utilisé pour l'extraction et l'analyse du texte. La langue" | |||
++ "par défaut du groupe est utilisée, si non spécifié" | |||
, scanRecursivelyInfo = "Recherchez également les courriels dans les sous-dossiers des dossiers donnés." |
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.
Rechercher également les mails dans les sous-dossiers des dossiers spécifiés.
@@ -311,6 +317,8 @@ fr tz = | |||
, documentLanguageInfo = | |||
"Utilisé pour l'extraction et l'analyse du texte. La langue" | |||
++ "par défaut du groupe est utilisée, si non spécifié" | |||
, scanRecursivelyInfo = "Recherchez également les courriels dans les sous-dossiers des dossiers donnés." | |||
, scanRecursivelyLabel = "Analyse récursive des dossiers" |
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.
ok for this one
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.
I 've commented inline for translation
Thanks @jgiradet
We have now tried implementing a first version for recursive mailbox scanning.
At the moment, it is controlled by a new checkbox in the Scan Mailbox Task settings.
This depends on our changes to emil (eikek/emil#293), but from our tests, it seems to work quite nicely now.
One problem we have left is how to handle database migration. The JSON parameters for scan-mailbox tasks is stored in a text field in the database, so adding a new "scanRecursively: false" parameter is probably not an option.
We thought the next logical thing was to simply make the new
scanRecursively
parameter optional, with a default value of false (previous behavior) - but that didn't work either, because then the JSON decoder complained that the field was missing. (Apparently, optional only means that it can benull
, but can not be simply missing...)