AleKoure / Rigma

R client to Figma API
https://alekoure.github.io/Rigma
Other
9 stars 2 forks source link

Simplify function internals by adding new `request_figma()` + `req_figma_query()` helper functions + add URL parsing features #9

Closed elipousson closed 11 months ago

elipousson commented 1 year ago

Hello @AleKoure! The package looks super handy and I'm looking forward to working with it. When I took a look at the code, I saw a few opportunities for refactoring to simplify the package internals and reduce duplicative code so I went ahead and made some updates.

I'm hoping this is a welcome contribution but happy to make adjustments if needed. The main additions are:

I also added a new feature allowing users to pass a Figma file URL in place of a file_key (the URL is parsed for a file_key). I also added this functionality for the project_id and I'm using it to make node_id optional. The input checking for the URL is now using cli. I added this to Imports but I figure it was already an upstream dependency so isn't a big lift.

Lastly, I replaced the deprecated call to invoke rlang::exec().

If your up for it, there are a few more changes I'd love to make:

Let me know what you think!

AleKoure commented 1 year ago

Hi @elipousson ! Changes look great 🚀 I agree that the package reached a state that internals need some refactoring!

I'll come back to this ASAP for review, since I was out of office.

Also, I'll try to open some issues so that things are more transparent since other parts of the package also need improvements at this stage (eg. code repetition, http testing, data tidying, documentation, and grinding deeper in some 'pro' features).

elipousson commented 12 months ago

Thanks for following up on this @AleKoure. I've been a little busy with work but really interested in getting back to this as soon as I can.

elipousson commented 12 months ago

Moving some scripts around means the requested changes are all flagged as "outdated" but I think all of the requested changes are now incorporated in the pull request. I also removed the row-binding purrr functions to avoid any issues with the planned deprecation. Let me know if you spot anything else that needs to be updated for this pull request and I'll comment separately with my thoughts on the URL parsing when I have a chance!

elipousson commented 11 months ago

Sounds great. I restored the *_dfr functions after struggling to get everything working and realizing there was no good reason to include them in this push request if it wasn't easy. I'll give everything a double-check over the weekend, add (or suggest the addition of) tests if needed, and (hopefully) merge.