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

[Elixir] Released package didn't include the gherkin-languages.json file #1293

Merged
merged 8 commits into from Jan 26, 2021

Conversation

WannesFransen1994
Copy link
Contributor

Missing file, moved to priv folder.

@WannesFransen1994 WannesFransen1994 marked this pull request as ready for review January 4, 2021 12:15
@WannesFransen1994
Copy link
Contributor Author

@aslakhellesoy , when you have time could you review this and publish a new version please? Thank you!

@alvivi
Copy link

alvivi commented Jan 20, 2021

Should I close #1281 then?

@WannesFransen1994
Copy link
Contributor Author

Oh wow sorry I completely missed your PR there! I'd personally suggest the default folder structure its priv folder. If everything is fine for @aslakhellesoy , this could be merged, published and it should be fixed then.

@alvivi
Copy link

alvivi commented Jan 20, 2021

Btw, @WannesFransen1994, Have you tried this patch when building/running a release? For our case, gherkin is being parsed in prod, no just dev/test environments. I'm not sure about the implications of using directly "priv" instead of :code.priv_lib/1.

@WannesFransen1994
Copy link
Contributor Author

Thank you for pointing that out @alvivi . I've played around with it on https://github.com/WannesFransen1994/ex_gherkin , it worked with just the "priv" (in prod, dev and test env) but as you stated, using :code.priv_dir/1 is definitely a better practice!

I've just looked at your PR a bit more, do you think that the @external_resource usage with decoding at compile time will result in a lot of performance increases? I personally didn't pay a lot attention to performance, so I'm really thankful that you mentioned it!

@alvivi
Copy link

alvivi commented Jan 20, 2021

Thank you for pointing that out @alvivi . I've played around with it on https://github.com/WannesFransen1994/ex_gherkin , it worked with just the "priv" (in prod, dev and test env) but as you stated, using :code.priv_dir/1 is definitely a better practice!

Yeah, for prod, test,etc. should be no problems. But I was talking about running it inside a release. Sorry for the misunderstanding. As far I know, the priv directory is copied in the release, at least of the current project. But not sure about dependencies 🤞

I've just looked at your PR a bit more, do you think that the @external_resource usage with decoding at compile time will result in a lot of performance increases? I personally didn't pay a lot attention to performance, so I'm really thankful that you mentioned it!

Is going to be faster and increase the memory footprint a little bit for sure (well, that memory is going to be used anyway). Not sure for this use case is going to make a difference or not (Is just bootstrap time). But no, the main reason for me was to ease distribution. I think this is a common approach for immutable data, for example, https://github.com/elixir-plug/mime (which includes a priv folder, but it is been read at compile time).

@WannesFransen1994
Copy link
Contributor Author

Just confirmed that running it in a release isn't an issue. In the release map, the dependencies their priv folder is copied as well to that dependency its subfolder.

Did stumble upon an error though, regarding the elixir_uuid library. When building a release, :crypto isn't included and will raise an error that the function doesn't exist. Simply adding this to its mix.exs extra_applications section resolved it for me. I'll make an issue for that.

I see your point. As you stated, the memory is going to be used anyway so I'll adjust it as well. Thanks for taking the time to explain this!

@WannesFransen1994 WannesFransen1994 merged commit 5524d4f into master Jan 26, 2021
@WannesFransen1994 WannesFransen1994 deleted the elixir-gherkin-bugfix branch January 26, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants