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

added 42Istanbul #452

Closed
wants to merge 6 commits into from
Closed

added 42Istanbul #452

wants to merge 6 commits into from

Conversation

bertt6
Copy link

@bertt6 bertt6 commented May 28, 2023 •
edited
Loading

Describe the pull request

Checklist

  • I have made the modifications or added tests related to my PR
  • I have run the tests and linters locally and they pass
  • I have added/updated the documentation for my RP

Additional context

@bertt6 bertt6 requested a review from 42atomys as a code owner May 28, 2023 22:24
@42atomys
Copy link
Owner

@bertt6 hello, thanks for your contribution, you forgot to run linters on your contribution (logs: https://github.com/42Atomys/stud42/actions/runs/5106707296/jobs/9179074008?pr=452)

Please run it yarn make-pretty to auto format your code.
yarn style:all to check of your modifications is aligned with the project :)

@42atomys 42atomys linked an issue May 28, 2023 that may be closed by this pull request
feat: integrate istanbul campus #339
Open
1 task
@pull-request-size pull-request-size bot temporarily deployed to previews May 29, 2023 22:10 Inactive
@pull-request-size pull-request-size bot temporarily deployed to previews May 29, 2023 22:10 Inactive
42atomys
42atomys requested changes May 30, 2023
View reviewed changes
Copy link
Owner

@42atomys 42atomys left a comment

Choose a reason for hiding this comment

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

Good job 🎉

Few more things needs to be done before merge !

web/ui/package-lock.json Outdated Show resolved Hide resolved
web/ui/package.json Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

This upgrade is not relative to the pull request, can you revert this change to follow the pull request scope ? :)

Copy link
Author

Choose a reason for hiding this comment

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

Actually i don't understand what you want. Can you explain a little more?

Copy link
Owner

Choose a reason for hiding this comment

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

You have done an update of few packages on package.json file, and this is not relative to add istanbul scope,

Due to this update a very huge change is added to the PR Screenshot 2023-05-31 at 12 41 21

This not be inside of this pull request (add istanbul =/= update dependencies). You need to revert this changes to only keep changes relative to add istanbul scope

@42atomys 42atomys added type/improvement ✨ Improvement to an existing feature state/needs information 🚧 We needs more information to go forwards aspect/interface 🕹 Concerns end-users' experience with the software domain/obvious 🟩 Represents the "known knowns" issue. It's Obviously labels May 31, 2023
@bertt6
Copy link
Author

bertt6 commented Jun 3, 2023

If i don't use yarn install i can't "yarn make-pretty". So I have to write "yarn install". And if i write "yarn install", yarn is adding and changing a lot of file.

@pull-request-size pull-request-size bot temporarily deployed to previews June 3, 2023 21:48 Inactive
@pull-request-size pull-request-size bot temporarily deployed to previews June 3, 2023 21:48 Inactive
@42atomys
Copy link
Owner

42atomys commented Jun 3, 2023

@bertt6 I encourage you to read and follow the contributing file https://github.com/42Atomys/stud42/blob/main/CONTRIBUTING.md all things to know is listed on it :)

@bertt6
Copy link
Author

bertt6 commented Jun 4, 2023

I have already read. So, what should i do right now?

@42atomys
Copy link
Owner

42atomys commented Jun 4, 2023

I think you don't use the devcontainer feature, you use the project directly on your host

@42atomys 42atomys force-pushed the main branch 3 times, most recently from cbd5be1 to 666d275 Compare September 9, 2023 15:16
@42atomys 42atomys removed the size/XL label Mar 31, 2024
@42atomys
Copy link
Owner

42atomys commented May 8, 2024

Due to a long time of inactivity, this pull request has been closed.
Create a new one to implement your changes again

Best regards!

@42atomys 42atomys closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@42atomys 42atomys 42atomys requested changes

Assignees

@bertt6 bertt6

Labels
aspect/interface 🕹 Concerns end-users' experience with the software domain/obvious 🟩 Represents the "known knowns" issue. It's Obviously state/needs information 🚧 We needs more information to go forwards type/improvement ✨ Improvement to an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: integrate istanbul campus
2 participants
@bertt6 @42atomys