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

Update README.md #1546

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Update README.md #1546

merged 2 commits into from
Nov 2, 2023

Conversation

AkshayJangid
Copy link
Contributor

@AkshayJangid AkshayJangid commented Oct 31, 2023

by going through the documentation , it is no where hinted that we need to place Types declaration in separate file. Thus explicitly mentioning it in readme.

Updated Readme file to explicitly mention about adding Types declaration in separate file.

Description

By going through the documentation it is not very clear about adding this in separata file. This leads to error
Cannot read properties of undefined on the leaf elements.

Related Issue

Motivation and Context

I was stuck because of this for 4 hours.

How Has This Been Tested?

Not required.

Types of changes

  • [x ] Updated docs / Refactor code / Added a tests case (non-breaking change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • [x ] I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the changelog.

by going through the documentation , it is not where hinted that we need to place it in separate file. Thus explicitly mentioning it in readme.
@Jameskmonger
Copy link
Member

Hi @AkshayJangid , thanks for the submission - I don't think this is intended behaviour but it would be good to document it while it's still present. I'm just catching up on #1455. It might be good to link to this issue in the README, as my understanding is that this is a bug and should be fixed.

@AkshayJangid
Copy link
Contributor Author

Hey @Jameskmonger , the problem is at least 4 people i know were stuck because of this for a day. Hence just trying to add it temporarily.

@Jameskmonger
Copy link
Member

@AkshayJangid I completely agree. Please adjust the PR to have:

bug: PLEASE MAKE SURE TO PLACE THIS TYPES DECLARATION IN A SEPARATE FILE. (see bug #1455)

then I will merge it immediately. Ty

add link to bug
@Jameskmonger
Copy link
Member

Looks like I can edit your PR 😄 I will merge now, ty for contrib @AkshayJangid

@codecov-commenter
Copy link

Codecov Report

Merging #1546 (ba0da86) into master (05a9237) will not change coverage.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master    #1546   +/-   ##
=======================================
  Coverage   99.48%   99.48%           
=======================================
  Files          53       53           
  Lines        1370     1370           
  Branches      200      200           
=======================================
  Hits         1363     1363           
  Partials        7        7           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jameskmonger Jameskmonger merged commit 447e71f into inversify:master Nov 2, 2023
9 checks passed
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.

3 participants