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.

Sign up for GitHub

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

Jump to bottom

Simplify parsing and printing attributes of function and arguments #5783

Merged
merged 7 commits into from
Nov 7, 2022
Merged

Simplify parsing and printing attributes of function and arguments #5783

merged 7 commits into from
Nov 7, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Nov 7, 2022 •
edited
Loading

Moved rescript-lang/syntax#683

Regarding issue #5735

@mununki mununki changed the title parse attributes for function and arg respectively Simplify parsing and printing attributes of function and arguments Nov 7, 2022
@mununki
Copy link
Member Author

mununki commented Nov 7, 2022

I didn't add the build artifacts of the compiler yet, but CI seems happy. Is it okay? @cknitt

cristianoc
cristianoc reviewed Nov 7, 2022
View reviewed changes
res_syntax/src/res_core.ml Outdated Show resolved Hide resolved
res_syntax/src/res_core.ml Show resolved Hide resolved
@cknitt
Copy link
Member

cknitt commented Nov 7, 2022

I didn't add the build artifacts of the compiler yet, but CI seems happy. Is it okay? @cknitt

CI does not enforce the snapshotting. It could also be done separately later.

Longer term I would like to have the snapshots removed from the sources, they are actually build artifacts IMHO, and they bloat the repo.

cristianoc
cristianoc reviewed Nov 7, 2022
View reviewed changes
res_syntax/CHANGELOG.md Outdated
@@ -13,6 +13,7 @@
#### :boom: Breaking Change

- Emit an error when a `@string` or `@int` attribute is used in a V4 component https://github.com/rescript-lang/rescript-compiler/issues/5724
- Parse the attributes of labelled argument to the pattern attributes of argument instead of function.
Copy link
Collaborator

@cristianoc cristianoc Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should change the compiler's changelog instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to compiler. 78206f4

@cristianoc cristianoc added this to the v11.0 milestone Nov 7, 2022
cristianoc
cristianoc approved these changes Nov 7, 2022
View reviewed changes
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

That's all I have for now.
After moving the log changes to the compiler log, I think it's ready to go.

@mattdamon108 anything else to check?

@mununki
Copy link
Member Author

mununki commented Nov 7, 2022

That's all I have for now. After moving the log changes to the compiler log, I think it's ready to go.

@mattdamon108 anything else to check?

No. This PR is ready.

@mununki
Copy link
Member Author

mununki commented Nov 7, 2022 •
edited
Loading

Side comment: The change log in the compiler seems to have something to be cleaned up(multiple breaking changes sections). But I wouldn't clean up in this PR. I don't want to make the CI run all the way again. 😃

@cristianoc cristianoc merged commit 645e9e0 into master Nov 7, 2022
@cristianoc cristianoc deleted the parse-attributes branch November 7, 2022 19:43
This was referenced Dec 14, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristianoc cristianoc cristianoc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mununki @cknitt @cristianoc