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

[WIP] Refactor convert_to_full_user_ids #121

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jknedlik
Copy link
Contributor

@jknedlik jknedlik commented Oct 6, 2024

Hi! Back for more:

As you can see in the main, I let the function consume the a.room_dm_create and reset. It is then consumed by into_iter and moved back at the end, which will not really have an impact on cpu.

The Iterator extensions in this file is a lot of boilerplate and will lay here only until I find out how to do it even better.

This project logs extensively and I still have to find out what the best approach to more clearly show the algorithms is.
I've, done so by moving the text above the function, but still keeping it near(L#306-L309) . @8go Would that be fine for you?

There also is this conundrum:

Why have you not filtered/error'ed the invalid user_ids but let the process continue? I understand the logging, but why not bubble up Errors there?

Cheers 😁

8go and others added 3 commits October 6, 2024 17:43
- fixed bug in --log-level : now RUST_LOG is appropriately used as default
- fixed bug --verify, --verify emoji improved, --verify emoji-req works on Element Web app but still does not work on Element Android app
@jknedlik jknedlik marked this pull request as draft October 6, 2024 16:25
@jknedlik jknedlik marked this pull request as ready for review October 6, 2024 16:25
@jknedlik jknedlik marked this pull request as draft October 6, 2024 16:26
@8go
Copy link
Owner

8go commented Oct 21, 2024

There also is this conundrum:

Why have you not filtered/error'ed the invalid user_ids but let the process continue? I understand the logging, but why not bubble up Errors there?

I did that to be super extra safe. "It will fail, but let's try anyway". I think it is a valid strategy, to bubble up the Error immediately.

@8go
Copy link
Owner

8go commented Oct 21, 2024

Does this remove the names that consist only of whitespaces (Space, Tabs, ...)? It should.

@8go
Copy link
Owner

8go commented Oct 21, 2024

in src/main.rs you removed 2 lines L#3738-L3739

didn't you want to remove the 2 lines L#3735-L3736 instead ?

Please look at this.

@8go
Copy link
Owner

8go commented Oct 21, 2024

I like it 👏

Consider my comments above. Make any needed changes and submit PR. I am happy to merge !

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.

2 participants