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

feat: add events #30

Merged
merged 3 commits into from
Feb 21, 2023
Merged

feat: add events #30

merged 3 commits into from
Feb 21, 2023

Conversation

Ananya2001-an
Copy link
Collaborator

πŸ‘¨β€πŸ’» Changes proposed

Created addEvents function

πŸ“„ Note to reviewers

Hey @Pradumnasaraf. I created the addEvents function. But there is one small issue that the date format is not the same as we want. After converting it into a date object we are getting something like this: 2023-08-09T00:00:00.000Z but it should be like this 2023-08-09T00:00:00.000+00:00 instead.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey! contributor, thank you for opening a Pull Request πŸŽ‰.

Soon one of our maintainers will review it and provide you with feedback/suggestions. If you think it's something urgent, feel free to reach out Pradumna Saraf on Twitter. Star ⭐ this repo to show us support.

Happy, Open Source!

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

Hey, Thanks for the PR.

About the issue. We should then go with talking the complete input from the user because this format might not work with the LinkFree, so there is a risk. Also, it will become simpler for us as well to handle, with no internal conversion

Later we can try to come up with some solution using some tool or API. For now, we can just put the sample format in the bracket when we prompt the message for data.

What is your take on this?

@Ananya2001-an
Copy link
Collaborator Author

Hey, Thanks for the PR.

About the issue. We should then go with talking the complete input from the user because this format might be with the LinkFree, so there is a risk. Also, it will become simpler for us as well to handle no internal conversion

Later we can try to come up with some solution using some tool or API. We can just put the sample format in the bracket when we prompt the message for data.

What is your take on this?

But isn't this difficult for a user to give because the date is fine but after that the timestamp part is something that the user should understand before writing anything

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

But isn't this difficult for a user to give because the date is fine but after that the timestamp part is something that the user should understand before writing anything

I agree with you 100%. This is why when we discussed about this, I said we need to make it easier at the user end (convert internally). But we have no option. We can't make JSON in the wrong format which will not work with LinkFree.

@Ananya2001-an
Copy link
Collaborator Author

But isn't this difficult for a user to give because the date is fine but after that the timestamp part is something that the user should understand before writing anything

I agree with you 100%. This is why when we discussed about this, I said we need to make it easier at the user end (convert internally). But we have no option. We can't make JSON in the wrong format which will not work with LinkFree.

Let me do one thing only the Z part at the end has to be replaced with something like +00:00 so I think I will keep this as default for now and later we can come up with a solution

@Pradumnasaraf
Copy link
Owner

But isn't this difficult for a user to give because the date is fine but after that the timestamp part is something that the user should understand before writing anything

I agree with you 100%. This is why when we discussed about this, I said we need to make it easier at the user end (convert internally). But we have no option. We can't make JSON in the wrong format which will not work with LinkFree.

Let me do one thing only the Z part at the end has to be replaced with something like +00:00 so I think I will keep this as default for now and later we can come up with a solution

Din't get you

@Ananya2001-an
Copy link
Collaborator Author

2023-08-09T00:00:00.000Z
this is the current output and what we want is that the Z at the end should be something like +00:00

@Pradumnasaraf
Copy link
Owner

Sure, you can have a look. But I suggest going with complete user input because date and time things are tricky.

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

Also, we are taking only the date as the input, not the time. That means time will always be 0 (i.e event will start mid night)

@Ananya2001-an
Copy link
Collaborator Author

Fine I think I will make the changes like I suggested before as time doesn't really matter that much

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

Fine I think I will make the changes like I suggested before as time doesn't really matter that much

No, it matters because we are adding events to the map which requires time to show nearby conferences happening at a given time.

@Ananya2001-an
Copy link
Collaborator Author

Okayyy that's a problem. Fine I will keep it as user input then.

@Pradumnasaraf Pradumnasaraf added enhancement New feature or request feature labels Feb 21, 2023
@Ananya2001-an
Copy link
Collaborator Author

Hey @Pradumnasaraf you can review the PR now. I have kept the date input completely upto user. No internal conversions done.

Copy link
Owner

@Pradumnasaraf Pradumnasaraf left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the changes. It's working great.

Small suggestions

In the prompt, we should add a little clear message that we are expecting both a date and time and also a sample for ease. Something like this

- message: "Give event start date",
+ message: "Give event start date and time. (Supported format: 2023-08-09T00:00:00.000+00:00),

@Ananya2001-an
Copy link
Collaborator Author

Done the changes @Pradumnasaraf

Copy link
Owner

@Pradumnasaraf Pradumnasaraf left a comment

Choose a reason for hiding this comment

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

Looks good to merge. Thank you for the changes.

Also, thanks for fixing that writter typo in other files

@Ananya2001-an
Copy link
Collaborator Author

Looks good to merge. Thank you for the changes.

Also, thanks for fr fix that writter typo in other files

No worries.

@Pradumnasaraf
Copy link
Owner

Should we merge it?

@Ananya2001-an
Copy link
Collaborator Author

Should we merge it?

yeah go ahead

@Pradumnasaraf Pradumnasaraf merged commit 4311d58 into Pradumnasaraf:refactor-cli Feb 21, 2023
@Ananya2001-an Ananya2001-an deleted the feat-add-events branch February 21, 2023 11:34
@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

Done

Amazing. All 4 major parts are done. Feels good and happy :)

@Ananya2001-an
Copy link
Collaborator Author

Done

Amazing. All 4 major parts are done.

Yeah finally. I think we have to just look into some small fixes here and there and then we are good to go!

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

Done
Amazing. All 4 major parts are done.

Yeah finally. I think we have to just look into some small fixes here and there and then we are good to go!

Yep, 100%. BTW a small shout out to you again. If you haven't opened the issues on the repo. I didn't move ahead and collaborate with you to fix this, I was not planning to do the CLI right now and even not 2.0.0. Just 1.5.0

Dang, I made so many typos 🀣 and edit again and again, sorry for that

@Ananya2001-an
Copy link
Collaborator Author

Done
Amazing. All 4 major parts are done.

Yeah finally. I think we have to just look into some small fixes here and there and then we are good to go!

Yep, 100%. BTW a small shout out to you again. If you haven't opened the issues on the repo. I didn't move ahead and collaborate with you to fix this, I was not planning to do the CLI right now and even not 2.0.0. Just 1.5.0

Dang, I made so many typos 🀣 and edit again and again, sorry for that

Thanks for saying that @Pradumnasaraf and I am also happy to work alongside you on this project. It was really fun. Kudos to u too for being a great maintainer πŸ‘

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

Thank you. I learned a lot from you. About structuring and code.

@Ananya2001-an
Copy link
Collaborator Author

Thank you. I learned a lot from out. About structuring and code.

Me too. I never worked on a cli tool before so it was nice to have a project that was easy to work with.

@Pradumnasaraf
Copy link
Owner

Thank you. I learned a lot from out. About structuring and code.

Me too. I never worked on a cli tool before so it was nice to have a project that was easy to work with.

Oh wow, I didn't know this was your 1st time on a CLI. Your contributors were huge πŸ‘

@Ananya2001-an
Copy link
Collaborator Author

Thank you. I learned a lot from out. About structuring and code.

Me too. I never worked on a cli tool before so it was nice to have a project that was easy to work with.

Oh wow, I didn't know this was your 1st time on a CLI. Your contributors were huge πŸ‘

haha fake it till u make it. I have learned that with time.

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

Also, forgot to ask you before, would you like to be a Maintainer?

@Ananya2001-an
Copy link
Collaborator Author

Also, forgot to ask you before, would you like to be a Maintainer?

Ohhh yessss I would love to. Thanks for asking @Pradumnasaraf This is an amazing project and I would love to be a part of it. 😁

@Pradumnasaraf
Copy link
Owner

Pradumnasaraf commented Feb 21, 2023

No, thank you for accepting. Sent you an invite :).

@Ananya2001-an
Copy link
Collaborator Author

No, thank you for accepting. Sent you an invite :).

Accepted! πŸ˜€

@Pradumnasaraf
Copy link
Owner

Awesome. Welcome πŸŽ‰ . This is the 1st time, I onboarded a maintainer on one of my projects.

@Ananya2001-an
Copy link
Collaborator Author

Awesome. Welcome πŸŽ‰ . This is the 1st time, I onboarded a maintainer on one of my projects.

Congrats to u too then πŸ‘

@Pradumnasaraf
Copy link
Owner

Awesome. Welcome πŸŽ‰ . This is the 1st time, I onboarded a maintainer on one of my projects.

Congrats to u too then πŸ‘

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants