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

[WIP] Add synthetic nav to http driver #743

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

slowmanchan
Copy link
Contributor

For Issue: #489

@codecov
Copy link

codecov bot commented Jun 5, 2022

Codecov Report

Merging #743 (dabe851) into master (80f21af) will decrease coverage by 0.0%.
The diff coverage is 0.0%.

❗ Current head dabe851 differs from pull request most recent head edfc6af. Consider uploading reports for the commit edfc6af to get more accurate results

@@           Coverage Diff            @@
##           master    #743     +/-   ##
========================================
- Coverage    35.2%   35.2%   -0.0%     
========================================
  Files         354     354             
  Lines       13062   13070      +8     
========================================
  Hits         4604    4604             
- Misses       8019    8027      +8     
  Partials      439     439             

@ziflex
Copy link
Member

ziflex commented Jun 6, 2022

Hey,

thanks for your work on this item!
There is a problem with your approach but it's not your fault :)

Here is a domain hierarchy of packages:

  • compiler
    • standard library
      • drivers
        • runtime

The access must be from the top to the bottom. Not vice versa.
In your case, you are accessing the standard library from http driver which breaks the access flow rule.

What you need to do instead is reuse existing functionality in the driver itself.

@slowmanchan
Copy link
Contributor Author

Stupid question but, what is the purpose of having this sort of heirarchy?

@slowmanchan
Copy link
Contributor Author

Can I get a review on this PR when you have the chance.

@slowmanchan slowmanchan closed this Sep 5, 2022
@slowmanchan slowmanchan reopened this Sep 5, 2022
@slowmanchan
Copy link
Contributor Author

@ziflex just lookin for a review of this PR when you have time!

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.

2 participants