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: CacheFS #18

Merged
merged 7 commits into from
Feb 7, 2023
Merged

feat: CacheFS #18

merged 7 commits into from
Feb 7, 2023

Conversation

pawelhertman
Copy link
Contributor

@pawelhertman pawelhertman commented Feb 6, 2023

What did you implement:

An abstraction over Roku's native CacheFS caching mechanism

How did you implement it:

CacheFS prototype-based object + Roku's EVPDigest component with mock

How can we verify it:

Run unit tests

Todos:

  • Write documentation (if required)
  • Fix linting errors
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pawelhertman pawelhertman requested a review from a team as a code owner February 6, 2023 14:04
@pawelhertman pawelhertman requested review from tomska235 and kuldeepk-dazn and removed request for a team February 6, 2023 14:04
@pawelhertman pawelhertman changed the title feat: [CacheFS] feat: CacheFS Feb 6, 2023
Copy link
Contributor

@tomek-r tomek-r left a comment

Choose a reason for hiding this comment

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

Good job. Some improvement comments. I would like to see how you are going to compose keys.

src/components/CacheFS.brs Outdated Show resolved Hide resolved
prototype.write = function (key as String, data as Object) as Boolean
if (key = "") then return false

content = FormatJson(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add check if content = invalid return false. Thats because if data is corrupted FormatJson will return invalid. Not needed to store invalid (not sure if you can anyway)

Copy link
Member

Choose a reason for hiding this comment

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

In case of an error, FormatJson returns an empty string according to the documentation.
And from what I experienced.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm

FormatJson({s:})
Syntax Error. (compile error &h02) in $LIVECOMPILE(126)
invalid

are you sure @bchelkowski?

Copy link
Member

Choose a reason for hiding this comment

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

Is it from the console directly? You shouldn't be able to compile this code.
for example, this should return an empty string:
FormatJson({a: CreateObject("roSGNode", "Node")})

Copy link
Member

Choose a reason for hiding this comment

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

And to be honest I cannot think of a case where you will receive a corrupted data argument.
Without a crash

Copy link
Contributor

Choose a reason for hiding this comment

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

@bchelkowski

And to be honest I cannot think of a case where you will receive a corrupted data argument.
Without a crash

From docs:
An error will be returned if arrays/associative arrays are nested more than 256 levels deep.

Copy link
Contributor

@tomek-r tomek-r Feb 7, 2023

Choose a reason for hiding this comment

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

a,d should be valid only? c "null", f and g "null"? b "asd"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a safety check. Please take a look

Copy link
Contributor

Choose a reason for hiding this comment

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

answers to the quiz above:

a = "[]"
b = ""asd""
c = "null"
d = "1"
e = "null"
f = ""

Copy link
Member

Choose a reason for hiding this comment

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

@RadoslawZambrowski
The answer is... there is missing an "e" option 😄
Actually, it works as expected for JSON parser.

For more than 256 levels deep object - FromJson still returns an empty string. No invalid, or other error object.
Just prints in the logs that the error occurred.

Fun (or sad) fact: I needed to generate that JSON in JavaScript because BrightScript throws a stack overflow error when I use recurrence more than 177 times. Or maybe there is a limit of 180 backtrace lines.

pawelhertman and others added 2 commits February 7, 2023 11:14
Co-authored-by: Błażej Chełkowski <bchelkow@gmail.com>
Copy link
Contributor

@tomek-r tomek-r 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!

' @param {Object} data - any value acceptable by native FormatJson function except Invalid
' @returns {Boolean} false if data is not parseable or if storing failed
prototype.write = function (key as String, data as Object) as Boolean
if (key = "" OR data = Invalid) then return false
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if you can say that data that is Invalid, is not parseable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not saying this - that's why in docs I put any value acceptable by native FormatJson function except Invalid

Copy link
Member

Choose a reason for hiding this comment

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

ok, but in jsdoc you have
' @returns {Boolean} false if data is not parseable or if storing failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will add Invalid exception

if (key = "" OR data = Invalid) then return false

content = FormatJson(data)
if (content = "") then return false
Copy link
Member

Choose a reason for hiding this comment

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

Pretty much the same as Invalid data, it is hard to say if data, in that case, wasn't just an empty string,
so maybe we should return false, only if the content is an empty string but data is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not. If it were an empty string, it would be """". It's covered by a unit test

@bchelkowski bchelkowski merged commit ea21718 into getndazn:master Feb 7, 2023
github-actions bot pushed a commit that referenced this pull request Feb 7, 2023
# [2.3.0](v2.2.2...v2.3.0) (2023-02-07)

### Bug Fixes

* Fixed rules in eslintrc and fixed violations ([#16](#16)) ([092adde](092adde))

### Features

* Added support to missing Animation node fields ([#17](#17)) ([875785c](875785c))
* CacheFS ([#18](#18)) ([ea21718](ea21718))
@github-actions
Copy link

github-actions bot commented Feb 7, 2023

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

4 participants