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

Language specific FABLE_COMPILER defines #2653

Merged
merged 6 commits into from Dec 22, 2021

Conversation

dbrattli
Copy link
Collaborator

@dbrattli dbrattli commented Dec 10, 2021

Add language specific defines for FABLE_COMPILER. In addition to FABLE_COMPILER and FABLE_COMPILER_4 this PR will add one of the language specific below:

  • FABLE_COMPILER_PHP
  • FABLE_COMPILER_RUST
  • FABLE_COMPILER_DART
  • FABLE_COMPILER_PYTHON
  • FABLE_COMPILER_JAVASCRIPT
  • FABLE_COMPILER_TYPESCRIPT

it also adds

  • FABLE_COMPILER_4_OR_GREATER

What do you think @alfonsogarciacaro @ncave. Would something like this be OK?

@dbrattli dbrattli changed the title Language specific FABLE_COMPILER define Language specific FABLE_COMPILER defines Dec 10, 2021
Copy link
Member

@alfonsogarciacaro alfonsogarciacaro left a comment

Choose a reason for hiding this comment

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

This looks good to me. Could also a good opportunity to change FABLE_COMPILER_3 to FABLE_COMPILER_4 in beyond. Although dotnet does something like FABLE_COMPILER_3_OR_GREATER, maybe we should do the same?

@dbrattli
Copy link
Collaborator Author

Ok @alfonsogarciacaro adding the extra defines for FABLE_COMPILER_4 and FABLE_COMPILER_3_OR_GREATER. I was also a bit unure about FABLE_COMPILER_PY since we spell out PHP and RUST so just adding both variants for js, py and ts. Is it too much?

  • FABLE_COMPILER_PY and FABLE_COMPILER_PYTHON
  • FABLE_COMPILER_JS and FABLE_COMPILER_JAVASCRIPT
  • FABLE_COMPILER_TS and FABLE_COMPILER_TYPESCRIPT

- Will be overwritten by next release
@MangelMaxime
Copy link
Member

Code using compiler directives can already be confusing/hard to read without having duplicates of them.

I think, it is better to have a single version per language and to keep things consistent using the language full name seems better to me. If we use the short name/extension suffix there can be conflict/confusion between different languages.

@alfonsogarciacaro
Copy link
Member

I agree with @MangelMaxime, using just the fullname (e.g. FABLE_COMPILER_PYTHON) would likely be less confusing. I'm also more convinced now to use FABLE_COMPILER_4_OR_GREATER instead of FABLE_COMPILER_4 :)

@MangelMaxime
Copy link
Member

I am not sure about FABLE_COMPILER_4_OR_GREATER you can't guarantee that in Fable 5 this will be compatible.

In Haxe another transpiler which exist since a long time they just use haxe_3 and haxe_4 making the supported version explicit.

Because if we do FABLE_COMPILER_4_OR_GREATER then how can we target a specific version?

@@ -228,19 +228,14 @@ type Runner =
"FABLE_COMPILER"
"FABLE_COMPILER_3_OR_GREATER"
"FABLE_COMPILER_4"
"FABLE_COMPILER_4_OR_GREATER"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I was thinking we should just use FABLE_COMPILER_4_OR_GREATER and remove FABLE_COMPILER_4, do you think it's better to use both? If we use FABLE_COMPILER_4_OR_GREATER only at the end we should also change the other instances below.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would prefer FABLE_COMPILER_3 and FABLE_COMPILER_4 as we don't know if the code we are writing will really be supported with future version of Fable.

We only know that we support the currently existing version of Fable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm agnostic to this so that's why I added both options similar to .NET. But I will let you decide if I should remove one: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives#conditional-compilation

Copy link
Member

Choose a reason for hiding this comment

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

I didn't know that you where basing your proposition on how .NET does it.

For me FABLE_COMPILER_3 and FABLE_COMPILER_4 seems enough from what I saw in Haxe worlds.

But I understand if we want to stick to what .NET does if we feel it to be a good solution too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then I think we should probably stick with both FABLE_COMPILER_4 and FABLE_COMPILER_4_OR_GREATER. What do you think @alfonsogarciacaro Should we merge this as is?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're right @dbrattli! I thought .NET only used the X_OR_GREATER pattern 😅 I'd rather use only the X_OR_GREATER pattern for simplicity but it's true we likely should use both for consistency. In principle we should avoid breaking changes in future versions but we never know. As far as I know the only library using FABLE_COMPILER_3 is Fable.SimpleJson but this is because before it was relying on the internal JS representation of some types like Map, and this can indeed change in future versions (even minor versions) so it's not a good practice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, great. Let's try this. Fable.SimpleJson is an obvious candidate since we now have Fable.SimpleJson.Python. Fable.JsonProvider also btw have a single line that needs to be handled differently with different languages. I'll merge this PR now. Thanks for reviewing!!

@dbrattli dbrattli merged commit d400614 into fable-compiler:beyond Dec 22, 2021
@dbrattli dbrattli deleted the feature/lang-defines branch December 22, 2021 07:05
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.

None yet

3 participants