search

khaosdoctor / deno-opengraph

Fetches and parses data from website to get their OpenGraph metadata
MIT License
4 stars 2 forks source link

feat: add detailed type for meta tags #2

Open ooker777 opened 3 months ago

ooker777 commented 3 months ago

Requirements

fixes #1

ooker777 commented 3 months ago

Some of the standard metadata names don't actually have type string, or can be more specified than a generic string.

ooker777 commented 2 months ago

Thanks for your time to review it. In this commit I implement small suggestions, replace the utils.ts file with module Extract data from URLs to illustrate how I use this lib in practice. The module is just for illustration purpose, not to mean that I propose to add it to the lib, as I'm aware that it goes beyond the current scope. Do you think it's better to extend the scope a bit, or keep it as is?

ooker777 commented 2 months ago

Can you just please remove the formatting changes and the added modules? If you can add your module example here in the comments would be better because it wouldn't be mixed up with the review

As this is my first PR being actually reviewed in depth, I'm not sure what does your question mean and how to do it. Did you mean that I should temporarily remove the Extract data from URLs module so this PR about meta tags type is clean, then I can open a new PR with it included?

khaosdoctor commented 1 month ago

Yes, instead of adding the example module in the code, you can add the example code here in the comments, because it makes it a LOT easier to review, because it will only show the files you've actually changed.

Also, refrain from using your linter to change the formatting of the file, for example, using 4 spaces instead of 2, or including semicolons, because this will change lines where you haven't actually changed anything. If you're using VSCode, you can do this by using cmd + k + s or "Save without formatting"

ooker777 commented 1 month ago

I have removed the example code. I'm not sure what to do with the linter, as your original code also used 2 space indent

khaosdoctor commented 1 month ago

Thanks a lot! I’ll review it as soon as I can! Right now I’m in the middle of a business trip so I can’t give it much attention until about next week, but if I have an opening later on I’ll comment here!

One thing you can do now to speed it up is to resolve this conflict that appeared due to #4 which adds the title tag to the exported parameters

And thanks a lot for the contribution!

ooker777 commented 1 month ago

Something weird happened when I pulled the new version. First it said Merge with strategy ort failed because I had uncommitted files. I tried to commit them with git add . && git commit "commit name" and get:

error: pathspec 'commit name' did not match any file(s) known to git

I then split the command separately and this time it committed fine. And thus I pulled fine. I have no idea what happened.

The merge seems to have no conflict to me.

khaosdoctor commented 1 month ago

Can you please try to look up the files changed by #4 and see if the changes are in your local branch? If not, try doing git pull origin main —rebase (while in your branch) to sync your changes with the remove. If you do have any uncommitted files, first do a git stash then run the git pull origin main —rebase command and then git stash pop to get your changes back on the line.

In theory, according to GitHub (and it makes sense) you should have a conflicting file you both changed.

On 19 Jul 2024, at 16:34, Ooker @.***> wrote:

Something weird happened when I pulled the new version. First it said Merge with strategy ort failed because I had uncommitted files. I tried to commit them with git add . && git commit "commit name" and get:

error: pathspec 'commit name' did not match any file(s) known to git I then split the command separately and this time it committed fine. And thus I pulled fine. I have no idea what happened.

The merge seems to have no conflict to me.

— Reply to this email directly, view it on GitHub https://github.com/khaosdoctor/deno-opengraph/pull/2#issuecomment-2239323859, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYNMMAF7ESEJLSOZPUNS4TZNEPXHAVCNFSM6AAAAABI2YAPFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZZGMZDGOBVHE. You are receiving this because you commented.

ooker777 commented 1 month ago

Yes the code exists in my local. What I don't understand is that checking the file on GH (https://github.com/khaosdoctor/deno-opengraph/pull/2/files#diff-e8c3622ccfbed67f4b3058cd4417bf41d066236f49c6f3e63b7c243b584c6734) I don't see that code in the file. I have no idea.

(The error part is just me forgetting using the -m flag when committing.)

khaosdoctor commented 1 month ago

Well you seem to have fixed the conflict 😆 I'll review the rest asap