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

Update soqlDatatable.js to enhance $CurrentRecord api to handle null values #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

solo-1234
Copy link
Contributor

Here is my fix for #99
I tested all recipes in a scratch org with this change and they seem to be working correctly.

Keep me posted re: next steps - thank you!

@tsalb
Copy link
Owner

tsalb commented Jul 15, 2021

Hey @solo-1234 the placement of the logic separate to reference only datatypes is not ideal.

However, I see where you applied the potential fix. I have an idea that I could tackle in a month or two that would help based on your desired logic above.

I'll revisit this PR in due time and update you when I reach some testing conclusions on the corp fork!

Edit: As an aside - the commit message isn't detailed enough and follow good commit msg conventions. Not something to worry about now but just for future reference.

@solo-1234
Copy link
Contributor Author

I am new at this so thanks for the feedback! Should I look at old PRs for commit message conventions?

I am curious what you would do differently in terms of this fix so I"ll stay tuned.

Incidentally, I am thinking that you may want to use the nullish coalescing operator on types other than just lookup. For example, on text fields. I did some testing and think I see unintended behavior in recipes in a scratch org. Should I comment on them here / in the issue / start a new issue?

TY

@tsalb
Copy link
Owner

tsalb commented Jul 15, 2021

I don’t agree nullish coalescing is the right blanket approach. The ethos of this library is to “pass through” whatever the platform provides and I only want to sparingly interject.

No new issues, as I don’t see value there since there is already a PR (this one) with full context for this discussion.

On the topic of git commit messages - it’s both convention in this repo and just general “how to write a good commit message”. No specific recommendations on specific guides though.

Lastly, yep I’ll think about this (you’ve given enough breadcrumbs) but I want to harden some other features first before revisiting this one in a few months.

@tsalb
Copy link
Owner

tsalb commented Aug 6, 2021

Update on this - my bandwidth is not looking so good in Q3 so will revisit in Q4

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