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: add jams support #55

Closed
wants to merge 2 commits into from

Conversation

lonerapier
Copy link
Contributor

@lonerapier lonerapier commented May 19, 2022

Motivation

Hopes to Resolve #54 ;)

index.ts Outdated Show resolved Hide resolved
@lonerapier lonerapier force-pushed the feat/jams-support branch from 6a41fce to 4fd2ab3 Compare May 20, 2022 05:06
@lonerapier lonerapier requested a review from nmushegian May 20, 2022 05:07
index.ts Outdated Show resolved Hide resolved
@lonerapier lonerapier force-pushed the feat/jams-support branch from 4fd2ab3 to f1d3b2b Compare May 20, 2022 09:17
@lonerapier lonerapier requested a review from nmushegian May 20, 2022 09:18
@nmushegian
Copy link
Member

Good now let’s add a test, I know we didn’t have one but that’s no excuse haha

@lonerapier
Copy link
Contributor Author

Good now let’s add a test, I know we didn’t have one but that’s no excuse haha

Yup, that's what I was gonna ask. But current tests aren't working with this change.

Still getting module not found errors, when running tests.

@dmfxyz
Copy link

dmfxyz commented May 21, 2022

@dsam82 I cloned your fork and was able to get tests passing locally without any changes. are you still running onto this issue?

@lonerapier lonerapier force-pushed the feat/jams-support branch from f0b959a to 32378ac Compare May 21, 2022 05:47
@lonerapier
Copy link
Contributor Author

lonerapier commented May 21, 2022

@dsam82 I cloned your fork and was able to get tests passing locally without any changes. are you still running onto this issue?

Unfortunately, yes. Have still added the tests, but can't remove this stupid error on my system.

arg = (isCid(arg)) ? await getIpfsJson(arg) : require(arg)
if (isCid(arg)) {
arg = getIpfsJson(arg)
} else if (arg.split('.').pop() === 'jams') {
Copy link
Member

Choose a reason for hiding this comment

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

This 3-way branch is not right, we have 2x2 options,

  • Are we using a CID and fetching it from IPFS, or opening a local file?
  • Is it JAMS or JSON?

Copy link
Contributor Author

@lonerapier lonerapier May 21, 2022

Choose a reason for hiding this comment

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

Does this work?

if (isCid(arg)) {
  arg = getIpfsJson(arg)
} else {
 arg = (arg.split('.').pop() === 'jams')
    ? jams(readFileSync(arg))
    : require(arg)
}

Copy link
Member

Choose a reason for hiding this comment

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

This one is missing the case that it is a CID that points to a JAMS file, maybe refactor getIpfsJson to just getIpfsFile then branch from there, point is there are 4 cases

@stobiewan
Copy link
Contributor

@dmfxyz Were you on the feat/jams-support branch when you had tests passing? I get the same failures

@dmfxyz
Copy link

dmfxyz commented May 31, 2022

@dsam82 I cloned your fork and was able to get tests passing locally without any changes. are you still running onto this issue?

Ignore this -- I was on the wrong branch.

@lonerapier lonerapier closed this Jun 12, 2022
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.

support jams
5 participants