-
Notifications
You must be signed in to change notification settings - Fork 42
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
Log events #110
Log events #110
Conversation
# Conflicts: # package.json # src/main.ts # yarn.lock
src/components/app/MainContent.vue
Outdated
data() { | ||
setup() { | ||
const inProgress = ref(false); | ||
const succeeded = ref('unknown' as keyof typeof colors); |
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 doesn't ever change. Don't think this was what you intended.
Need to add back the logic that updates according to result of scraping
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.
In general I think we should replace succeeded and in progress with a single status. Something like:
status: NOT_STARTED | IN_PROGRESS | SUCCESS | FAILURE
Then colors becomes STATUS_COLORS: Record<ScrapingStatus, string | null>
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 can actually use the event emitter for setting the status instead of awaiting the promise (not a must):
eventPublisher.on(EventNames.IMPORT_PROCESS_START, () => {
// IN_PROGRESS
});
eventPublisher.on(EventNames.IMPORT_PROCESS_END, () => {
// SUCCESS
});
eventPublisher.on(EventNames.GENERAL_ERROR, () => {
// 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.
In general I think we should replace succeeded and in progress with a single status. Something like:
status: NOT_STARTED | IN_PROGRESS | SUCCESS | FAILUREThen colors becomes STATUS_COLORS: Record<ScrapingStatus, string | null>
I think I don't understand... You just suggesting to change the unknown | success | fail
with these NOT_STARTED | IN_PROGRESS | SUCCESS | FAILURE
?
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 can actually use the event emitter for setting the status instead of awaiting the promise (not a must):
eventPublisher.on(EventNames.IMPORT_PROCESS_START, () => {
// IN_PROGRESS
});eventPublisher.on(EventNames.IMPORT_PROCESS_END, () => {
// SUCCESS
});eventPublisher.on(EventNames.GENERAL_ERROR, () => {
// ERROR
});
As I said before, I don't like to use EventEmitter
as a "serial" flow, because I think it is less readable.
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.
Discussed on the phone. Easier to communicate that way 🙂
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 did it. I started with enum
but I think for now we can just use the keyof typeof statusToColor
I don't fully understand what you mean. You could either listen with I am not saying this implementation is optimal, I am just not sure what are the actual pain points so we can improve the current design. We can discuss this on the phone if you want. |
I can't tell you exactly what is the pain. Of course, I can listen to a specific event to get the specific type. But I want it to be generic. Also, take the In short, I think the better is to let you try to improve my code, if you want and if you have time, and see if it hurts you. |
Thats fine. As I said we will have a real use case once we implement the progress indication. For now I suggest just the changes in the comments |
@brafdlog Ready, waiting for your approval. |
Move DataTable from ElementUI to Vuetify
Using the
EventEmitter
, I'm printing the events to the UI, to see the process and errors if any.It's very hard to work with the current
EventEmitter
design. You have to declare manually all theEventNames
, and you don't have type-checking for that.You can achieve type-checking, of course, but it creates a very complicated code.
I think we need to define the events as classes instead of interfaces, and yes, it will be noises in the backend, so maybe create a helper method to create these objects.
Anyway, don't know. We need more helper methods here.