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(presence): implemented presence function #2123

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tauantcamargo
Copy link
Contributor

Implemented presence function.

Issue: #654

@tauantcamargo tauantcamargo mentioned this pull request Dec 27, 2021
@char0n char0n self-assigned this Jan 2, 2022
@char0n char0n added the feature label Jan 2, 2022
Copy link
Owner

@char0n char0n left a comment

Choose a reason for hiding this comment

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

Thanks for contributing,It's looking good so far!. The PR must usually come with 5 files modified. You can see an example in this PR. This PR is missing typings and typings tests. Also please have a look at my code review comments.

src/presence.js Outdated Show resolved Hide resolved
src/presence.js Outdated Show resolved Hide resolved
src/presence.js Outdated Show resolved Hide resolved
src/presence.js Outdated Show resolved Hide resolved
src/presence.js Outdated Show resolved Hide resolved
src/presence.js Outdated Show resolved Hide resolved
src/presence.js Show resolved Hide resolved
src/presence.js Outdated Show resolved Hide resolved
test/presence.js Outdated Show resolved Hide resolved
@tauantcamargo
Copy link
Contributor Author

@char0n i'll take a look to the requests, thank u by the way.

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #2123 (1810924) into master (6c82686) will not change coverage.
The diff coverage is n/a.

❗ Current head 1810924 differs from pull request most recent head 191baf5. Consider uploading reports for the commit 191baf5 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2123   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         234      234           
  Lines         622      622           
=======================================
  Hits          612      612           
  Misses         10       10           
Flag Coverage Δ
unittests 98.39% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c82686...191baf5. Read the comment docs.

@char0n
Copy link
Owner

char0n commented Jan 23, 2022

I'm not sure I understand changes that have been made to this PR during last 3 commits. master branch now has isBlank function which is part of the public API of ramda-adjunct. We just cannot move it to internal directory anymore.

Now we should also fully utilize isBlank implementation to it's full potential:

const presence = when(isBlank, stubNull);

Notes to above implementation:

  • present is opposite of blank
  • when value is present return it, otherwise return null
  • the above sentence can also be: when value is blank return null, othersiwe return the original value (and that's our implementation)

@tauantcamargo
Copy link
Contributor Author

@char0n i got it .. i'll update it later ..

@char0n
Copy link
Owner

char0n commented Feb 5, 2022

Right, let me know if you need any assistence

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