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

space-after-if-condition #39

Merged
merged 4 commits into from
Sep 25, 2018
Merged

space-after-if-condition #39

merged 4 commits into from
Sep 25, 2018

Conversation

mattiaerre
Copy link
Member

@mattiaerre mattiaerre commented Sep 21, 2018

Description

@codecov
Copy link

codecov bot commented Sep 21, 2018

Codecov Report

Merging #39 into master will increase coverage by 12.55%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #39       +/-   ##
===========================================
+ Coverage   52.22%   64.77%   +12.55%     
===========================================
  Files           4        4               
  Lines         157      159        +2     
  Branches       34       35        +1     
===========================================
+ Hits           82      103       +21     
+ Misses         61       49       -12     
+ Partials       14        7        -7
Impacted Files Coverage Δ
src/printer.js 61.37% <100%> (+13.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39cc037...127c362. Read the comment docs.

@mattiaerre
Copy link
Member Author

mattiaerre commented Sep 21, 2018

it fails if you run AST_COMPARE=true npm run test; can you help us w/ this @j-f1 ?

    Difference:

    - Expected
    + Received

    @@ -155,15 +155,16 @@
                  ],
                  "type": "Block",
                },
                "isConstructor": true,
                "modifiers": Array [],
    -           "name": null,
    +           "name": "BasicIterator",
                "parameters": Object {
                  "parameters": Array [],
                  "type": "ParameterList",
                },
    +           "returnParameters": null,
                "stateMutability": null,
                "type": "FunctionDefinition",
                "visibility": "default",
              },
              Object {

      66 |           expect(ppastMassaged).toBeDefined();
      67 |           if (!astMassaged.errors || astMassaged.errors.length === 0) {
    > 68 |             expect(astMassaged).toEqual(ppastMassaged);
         |                                 ^
      69 |           }
      70 |         });
      71 |       }

      at Object.test (tests_config/run_spec.js:68:33)

@mattiaerre mattiaerre added this to the beta version milestone Sep 21, 2018
@j-f1
Copy link

j-f1 commented Sep 22, 2018

Those are the kinds of issues that the AST-massaging function can handle.

@mattiaerre
Copy link
Member Author

mattiaerre commented Sep 23, 2018

not sure if I understand what you mean @j-f1 ; shall we change something here?

https://github.com/prettier-solidity/prettier-plugin-solidity/blob/master/tests_config/run_spec.js#L52-L71

@j-f1
Copy link

j-f1 commented Sep 23, 2018

I think so.

@@ -7,7 +7,7 @@ contract BasicIterator {
address creator; // reserve one "address"-type spot
uint8[10] integers; // reserve a chunk of storage for 10 8-bit unsigned integers in an array

function BasicIterator()
Copy link
Member Author

Choose a reason for hiding this comment

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

dah 😅 @j-f1 as we "translate" function ContractName => constructor

Copy link
Member

Choose a reason for hiding this comment

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

The plugin is doing that right now? I mean, converting function Contract to constructor? Because that shouldn't be prettier's responsibility IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

100% agree. Apparently it does that. See if I can fix it.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the parser adds the isConstructor metadata to functions named as the contract, and the printer function uses that to print constructor.

Copy link

@federicobond federicobond left a comment

Choose a reason for hiding this comment

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

LGTM

@mattiaerre
Copy link
Member Author

merging this and opening a new PR to try to fix #32

@mattiaerre mattiaerre closed this Sep 25, 2018
@mattiaerre mattiaerre reopened this Sep 25, 2018
@mattiaerre mattiaerre merged commit 78f5dc1 into master Sep 25, 2018
@mattiaerre mattiaerre deleted the space-after-if-condition branch September 25, 2018 18:28
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.

Put while opening brace in the same line Add space before if's opening braces
4 participants