Skip to content

Conversation

@maoo
Copy link
Member

@maoo maoo commented Aug 16, 2023

Preview available at https://website--endearing-brigadeiros-63f9d0.netlify.app/

Contents are available in the following files/folders:

  • docs
  • website/src/pages/index.js
  • website/src/components/

@netlify
Copy link

netlify bot commented Aug 16, 2023

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit b681733
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/64e4a6c9280ff30008ce76c7
😎 Deploy Preview https://deploy-preview-238--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@maoo maoo requested a review from JamieSlome August 16, 2023 14:01
@maoo
Copy link
Member Author

maoo commented Aug 16, 2023

@JamieSlome - apologies for forgetting, how can I make lint checks happy?

@JamieSlome
Copy link
Member

@vaibssingh - should npm run lint fix these issues?

@vaibssingh
Copy link
Contributor

@JamieSlome Not really. It only fixes the auto fixable problems. If it's not able to do so then those errors are highlighted in the console. User would get the exact same errors on their local too after running the command.

This workflow essentially fixes the lint issues if the user has forgot to run it before pushing and if there are no conflicting issues or issues that can't be auto fixed, this pipeline would always pass.

This actually makes me think that we should probably add this as a githook that runs before each push which would save time and efforts but then that would make this workflow redundant. Let me know what you think about this approach :)

@JamieSlome
Copy link
Member

@vaibssingh - thanks for the information. I will work with @maoo to address the lint issues.

Are we able to create a new issue that will lint the repository before a developer commits?

Appreciate your time as always @vaibssingh 👍

@JamieSlome
Copy link
Member

JamieSlome commented Aug 21, 2023

@vaibssingh - I've just checked out onto the website branch, and it seems like there is a conflict between ESLINT and Prettier. When I try and save a file that is complaining about max-len, it doesn't auto-adjust the code.

I wonder if we need a .prettierrc which will consistently define the rules for making the code "pretty".

@JamieSlome
Copy link
Member

@maoo @TheJuanAndOnly99 - are we able to disable the linting rule for now whist we understand what is going on. We can then merge this PR, and then look to lint/pretty the entire codebase after 👍

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

@maoo - this is my first time using Docusaurus. Should the website/.docusaurus file changes be included in this PR?

I can see that you have included this file path in the .gitignore?

@maoo
Copy link
Member Author

maoo commented Aug 21, 2023

@maoo - this is my first time using Docusaurus. Should the website/.docusaurus file changes be included in this PR?

I can see that you have included this file path in the .gitignore?

.docusaurus should definitely be in .gitignore , sorry if it slipped through.

@vaibssingh
Copy link
Contributor

Hey @JamieSlome , I think I might have missed that rule while adding the config. Sorry about that. Let me have a look at it.

I agree with you on adding a .prettierrc file. At this point instead of cramming all the exceptions in the eslint file, that would be a better option.

On the topic of updating the rules, I am also going to add a rule that doesn't allow var as a keyword and forces use of let and const which I had missed previously.

@vaibssingh
Copy link
Contributor

@JamieSlome meanwhile I have opened #241 which I am happy to take up.

Should I create another issue for adding the .prettierrc file and modifying the existing lint rules or would it be okay if I include those changes in #241 ? Let me know how you want me to go about it.

@vaibssingh
Copy link
Contributor

@JamieSlome I have created another PR #243 against this branch which fixes the above issues. This does not include any changes in eslint config or rules. Most of these fixes are manual and some automatically added once the manual issues were resolved. Let me know if this works for you. This should get this PR to pass now.

The only thing I request you to verify is this line here:
https://github.com/finos/git-proxy/pull/243/files#diff-407e2461e3a4cecc5674cfb270017d108ec3c133040ae77edcce63f0feddde1eR20

I tried it on my machine but I can't get past the login screen so I can not check how does this render on browser.

fix: lint errors for website branch fix
@JamieSlome
Copy link
Member

@vaibssingh - merged! 🎉

Feel free to include all changes in #241. I'd say get the linting and prettier into a position we are happy with on one branch, and then we can merge into main.

Appreciate you @vaibssingh 👍

@JamieSlome
Copy link
Member

@maoo - can we look at what is causing the Netflify checks to fail now? @vaibssingh has kindly addressed the linting issues for us!

@maoo
Copy link
Member Author

maoo commented Aug 22, 2023

@maoo - can we look at what is causing the Netflify checks to fail now? @vaibssingh has kindly addressed the linting issues for us!

Issue is:

10:46:50 AM: ReferenceError: PropTypes is not defined

I suppose it can be reproduced with a local yarn install ; yarn build.

@vaibssingh
Copy link
Contributor

@vaibssingh - merged! 🎉

Feel free to include all changes in #241. I'd say get the linting and prettier into a position we are happy with on one branch, and then we can merge into main.

Appreciate you @vaibssingh 👍

Got it

@vaibssingh
Copy link
Contributor

vaibssingh commented Aug 22, 2023

@maoo - can we look at what is causing the Netflify checks to fail now? @vaibssingh has kindly addressed the linting issues for us!

Sorry about this one. Have opened another PR #245 that fixes the missing import. Somehow this edit got omitted during the commit. Hopefully this would be the last commit for this PR 😄

@vaibssingh
Copy link
Contributor

@vaibssingh - I've just checked out onto the website branch, and it seems like there is a conflict between ESLINT and Prettier. When I try and save a file that is complaining about max-len, it doesn't auto-adjust the code.

I wonder if we need a .prettierrc which will consistently define the rules for making the code "pretty".

@JamieSlome On this point I think we need little bit more clarification. The auto fixes don't work on a file as long as it has issues which have to be fixed manually . Most of the indenting, quotes and similar issues get fixed automatically but the errors where the code needs to be changed in any meaningful way are not fixed.

Whether the issue can be fixed automatically or not, depends on what exactly is the issue with the code. Usually the max-len issue gets fixed automatically in cases of chained operations like DB query etc by breaking its different stages on new lines. But in this particular case, the variables had to be moved around (or alternatively breaking the line in a sensible manner) which it can not do on its own and hence the persistent error.

I agree on the need for a new .prettierrc file which I will address in #241 but I don't think there is a conflict here in the rules. Let me know if I have missed anything or maybe the behaviour is different on your system which we can figure out if that's the case.

@JamieSlome
Copy link
Member

All checks passing, waheey! 🎉

Thanks, @vaibssingh!

@maoo, can we remove the files and folders that shouldn't be included in this PR, append them to the .gitignore if needed, and then we can merge the PR?

@maoo
Copy link
Member Author

maoo commented Aug 22, 2023

All checks passing, waheey! 🎉

Thanks, @vaibssingh!

@maoo, can we remove the files and folders that shouldn't be included in this PR, append them to the .gitignore if needed, and then we can merge the PR?

done!

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

@JamieSlome
Copy link
Member

@vaibssingh - I've just checked out onto the website branch, and it seems like there is a conflict between ESLINT and Prettier. When I try and save a file that is complaining about max-len, it doesn't auto-adjust the code.
I wonder if we need a .prettierrc which will consistently define the rules for making the code "pretty".

@JamieSlome On this point I think we need little bit more clarification. The auto fixes don't work on a file as long as it has issues which have to be fixed manually . Most of the indenting, quotes and similar issues get fixed automatically but the errors where the code needs to be changed in any meaningful way are not fixed.

Whether the issue can be fixed automatically or not, depends on what exactly is the issue with the code. Usually the max-len issue gets fixed automatically in cases of chained operations like DB query etc by breaking its different stages on new lines. But in this particular case, the variables had to be moved around (or alternatively breaking the line in a sensible manner) which it can not do on its own and hence the persistent error.

I agree on the need for a new .prettierrc file which I will address in #241 but I don't think there is a conflict here in the rules. Let me know if I have missed anything or maybe the behaviour is different on your system which we can figure out if that's the case.

You have summarised perfectly - I did get auto-fixing issues and some remained. Let's focus on #241 and appreciate the clear delineation of the issues here. Nice work! 👍

@JamieSlome
Copy link
Member

@maoo - will this be available under https://git-proxy.finos.org?

@maoo
Copy link
Member Author

maoo commented Aug 22, 2023

@maoo - will this be available under https://git-proxy.finos.org?

https://git-proxy.finos.org/ is up - but please do not share it yet (other than for preview), I believe we need to do few more iterations to make it look nice. SSL will be enabled in the next few minutes.

@JamieSlome
Copy link
Member

@maoo - of course, no problem 👍

@JamieSlome JamieSlome merged commit 375ef24 into main Aug 23, 2023
@JamieSlome JamieSlome deleted the website branch August 23, 2023 15:58
@coopernetes
Copy link
Contributor

@maoo can we open an issue to create a workflow to publish the doc site through CI?

@maoo
Copy link
Member Author

maoo commented Sep 18, 2023

@maoo can we open an issue to create a workflow to publish the doc site through CI?

Hi @coopernetes ! We're using Netlify, a CDN that pulls code, runs the build and publishes the site for you; it also provides a preview on Pull Requests; no need for extra CI steps 😄

coopernetes pushed a commit to coopernetes/git-proxy that referenced this pull request Oct 13, 2023
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.

4 participants