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

Added support for Birb #2542

Merged
merged 6 commits into from
Sep 11, 2020
Merged

Conversation

Calamity210
Copy link
Contributor

Added support for a custom language(Birb)

@RunDevelopment
Copy link
Member

Thank you for making this language definition @Calamity210!

However, I don't know if we can merge this.
My main problem is that there is no language specification or detailed documentation for Birb. Without such a document, I have no way of knowing that your language definition is actually correct.

Right now, the documentation for your language is too little for others to work with. Specifically for this PR, the doc is inconsistent with the Prism language definition. E.g. at no point does the doc mention triple-quote string literals and the doc explicitly states that single-quote string literals can be multiline but that isn't supported by the language definition.

The lack of documentation also means that the language definition will be practically unmaintainable since we won't be able to review any patches or enhancements to the Birb language definition either.

Thoughts? @Calamity210 @mAAdhaTTah

@Calamity210
Copy link
Contributor Author

Calamity210 commented Sep 7, 2020

For sure makes sense to me, the triple quotes was actually incorrect, Birb doesnt support it. Mind if I request a review from you once our docs are improved? @RunDevelopment

@Calamity210 Calamity210 marked this pull request as draft September 7, 2020 15:02
@RunDevelopment
Copy link
Member

Sure!

(Maybe add a comment with a link to the docs to your language definition once you're finished.)

@Calamity210
Copy link
Contributor Author

@RunDevelopment We are still working on an api documentation, but the tour documents the complete syntax. https://birbolang.web.app/docs/ this document contains more or less everything about Birbs syntax itself as it stands right now, what exactly are you looking for with the docs?

@RunDevelopment
Copy link
Member

what exactly are you looking for with the docs?

Prism highlights concepts (functions, classes, operators, ...), so when reading through language documentation, I'm always looking for all the concepts the language has to offer. I then go through each concept and look for its syntax (most docs do this via examples but some even give you the BNF grammar of single concepts or even the whole language).
To summaries: I look for everything the language can do and how that is expressed in code.

I think the doc is ok in that regard. One of the nice sides of a minimal language is that you don't need a lot of documentation to describe it.

Ready for review?


Also, I'm a little curious as to how Birb infers the type of a variable with var. Example:

var a = 100; // clearly int
var b = 0.5; // clearly double
var c = 1e2; // int or double?
var d = 1e100; // too big for int, so is it a double?

@Calamity210
Copy link
Contributor Author

Calamity210 commented Sep 11, 2020

Ready for review?

Yeah, lets do it!

Also, I'm a little curious as to how Birb infers the type of a variable with var. Example:

var a = 100; // clearly int
var b = 0.5; // clearly double
var c = 1e2; // int or double?
var d = 1e100; // too big for int, so is it a double?

var is still being worked on to better handle edge cases, but in the case you provided above, both c and d would be doubles. Exponential numbers in birb are doubles by default.

@RunDevelopment RunDevelopment marked this pull request as ready for review September 11, 2020 10:38
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Here's the review.

Apart from my comment, please use tabs for indentation. There are a few lines where tabs and spaces are mixed.

components/prism-birb.js Outdated Show resolved Hide resolved
components/prism-birb.js Outdated Show resolved Hide resolved
components/prism-birb.js Outdated Show resolved Hide resolved
components/prism-birb.js Outdated Show resolved Hide resolved
tests/languages/birb/string_feature.test Show resolved Hide resolved
tests/languages/java/function_featrue.test Outdated Show resolved Hide resolved
tests/languages/birb/operator_feature.test Outdated Show resolved Hide resolved
components/prism-birb.js Outdated Show resolved Hide resolved
tests/languages/birb/keyword_feature.test Outdated Show resolved Hide resolved
@Calamity210
Copy link
Contributor Author

Here's the review.

Apart from my comment, please use tabs for indentation. There are a few lines where tabs and spaces are mixed.

Had I mixed them, i had a bit of a brainfog the day I worked on this, ill correct this along with the comments in a bit. Thanks!

@Calamity210
Copy link
Contributor Author

@RunDevelopment I've taken a look at the comments and corrected them. I removed the pattern I had for functions as it seemed like I was misunderstanding something. Everything else should be fine afaict.

components/prism-birb.js Outdated Show resolved Hide resolved
components/prism-birb.js Show resolved Hide resolved
components/prism-birb.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

Looks good!

To resolve the merge conflicts, run npm run build after merging Prism's master branch and commit everything as is.

@Calamity210
Copy link
Contributor Author

Looks good!

To resolve the merge conflicts, run npm run build after merging Prism's master branch and commit everything as is.

Yup give me one min

@Calamity210
Copy link
Contributor Author

Resolved @RunDevelopment

@RunDevelopment RunDevelopment merged commit 4d31e22 into PrismJS:master Sep 11, 2020
@RunDevelopment
Copy link
Member

Thank you for contributing @Calamity210!

@Calamity210 Calamity210 deleted the birb-support branch November 26, 2020 00:05
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

2 participants