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

Gauge/bug fix #148

Merged
merged 27 commits into from
Sep 22, 2021
Merged

Gauge/bug fix #148

merged 27 commits into from
Sep 22, 2021

Conversation

Karan-S-Mittal
Copy link
Contributor

@Karan-S-Mittal Karan-S-Mittal commented Aug 29, 2021

These are the issues, I worked on

Before:

After:
chrome_3aFCeJTkLv

  • demo script missing #143
    changed yarn demo to npm run dash-demo
    This issue was related to documentation and mismatch between package.json commands. the contributing section of daq needed updation. I have changed the commands from yarn to npm as that proved for me to be less error-prone and all the commands were using npm. Please have a look at contributing section and documentation.

Thank you so much for reviewing.

@alexcjohnson
Copy link
Collaborator

Thanks @Karan-S-Mittal - the code changes look promising, I'll review those shortly, but something odd happened with the build process: the quote characters in the build:r package script were treated as part of the input, so we ended up with a bunch of duplicate files like R/'daq'Gauge.R in addition to the expected R/daqGauge.R, and man/'daq'Gauge.Rd in addition to man/daqGauge.Rd. Also in the new .Rd files there are a whole lot of ^M characters (CR) - not at all line breaks, but at many of them, and occasionally not at a line break at all, like here.

I'm assuming these are all a result of running the same commands we've used in the past, but you're on Windows and most of the rest of us develop on Mac or Linux. In a sense this is good, an opportunity for us: it should work fine to build components on Windows, we just need to figure out why it didn't.

The good news is building the Python components seems to have worked perfectly. There are changes in these files, but that's to be expected because we've updated the Python component generator since dash-daq was most recently rebuilt.

So the two routes we could take are:

  • Figure out a way to run on Windows with the existing code, and document this
  • Fix the code so it works correctly on Windows whichever way you run it.

The latter is preferable if you're up for it. The place this is all happening is https://github.com/plotly/dash/blob/dev/dash/development/_r_components_generation.py. Re: CR, if we're loading the source files correctly Python should have already converted \r\n to \n for us, but perhaps we need to explicitly replace os.linesep with \n in some cases? And re: the single quotes, maybe we want to solve this up a level? In principle all the args here should have quotes removed if any made it that far.

@Karan-S-Mittal
Copy link
Contributor Author

Thank you for sharing the build error, I have been searching on this.
I would like to work on making the repository more contributor-friendly on windows and try to make it more cross-platform. Again, Thank you for sharing the files with me, I will work on implement the changes as you suggested and update the PR.

@Karan-S-Mittal
Copy link
Contributor Author

Hey @alexcjohnson, #124 is resolved.

Before:

chrome_sc7qnQVD8Q

After:

issue 124

build error

this time I used Github's codespaces as a build environment(Linux) but for some reason, the build process is showing an error of unauthorized.
attaching an image of what I can see.

chrome_8ewmxXgcfo

@alexcjohnson
Copy link
Collaborator

Get rid of the context in circleci config - like https://github.com/plotly/dash-bio/pull/539/files
It's unnecessary and prevents PRs from forks from running CI tests.

@Karan-S-Mittal
Copy link
Contributor Author

Thanks @Karan-S-Mittal - the code changes look promising, I'll review those shortly, but something odd happened with the build process: the quote characters in the build:r package script were treated as part of the input, so we ended up with a bunch of duplicate files like R/'daq'Gauge.R in addition to the expected R/daqGauge.R, and man/'daq'Gauge.Rd in addition to man/daqGauge.Rd. Also in the new .Rd files there are a whole lot of ^M characters (CR) - not at all line breaks, but at many of them, and occasionally not at a line break at all, like here.

I'm assuming these are all a result of running the same commands we've used in the past, but you're on Windows and most of the rest of us develop on Mac or Linux. In a sense this is good, an opportunity for us: it should work fine to build components on Windows, we just need to figure out why it didn't.

The good news is building the Python components seems to have worked perfectly. There are changes in these files, but that's to be expected because we've updated the Python component generator since dash-daq was most recently rebuilt.

So the two routes we could take are:

  • Figure out a way to run on Windows with the existing code, and document this
  • Fix the code so it works correctly on Windows whichever way you run it.

The latter is preferable if you're up for it. The place this is all happening is https://github.com/plotly/dash/blob/dev/dash/development/_r_components_generation.py. Re: CR, if we're loading the source files correctly Python should have already converted \r\n to \n for us, but perhaps we need to explicitly replace os.linesep with \n in some cases? And re: the single quotes, maybe we want to solve this up a level? In principle all the args here should have quotes removed if any made it that far.

Hey Alex, I have checked the dash renderer and found that if we can add the strip function in this
https://github.com/Karan-S-Mittal/dash/blob/d5df9a704049169e29521e5c419973b9d16bd972/dash/development/_r_components_generation.py#L735-L738
we can resolve the files error.
here are some snapshots of before and after I made the change of file structure.

Before modification

WindowsTerminal_Mbc2LwgOOa

After the prefix variable is stripped for single quotes

WindowsTerminal_tRExTt4s2I

Though there are some platform issues with Dash Repository as well on windows and therefore cannot update the commit.
Can you please look at this simple addition and let me know if I am moving in the right direction.

image

The current build fails are a part of different errors that I wasn't able to track down. I am exploring and learning Circle CI more for that currently.

@Karan-S-Mittal
Copy link
Contributor Author

Issues Resolved in this PR update #113, #144, and thermometer #64.

Issue #113

the issue was to show the true value of the sensor which was being limited to the value to max attribute.
issue-113

Issue #144

the issue was similar to issue #113 but related to the tank.

  • [ x] Added two new props exceedMessage and lagingMessage which can be optionally provided by the user and will only trigger if the value exceeds the max limit or is lower than the minimum limit respetively.
    issue-144

Issue #64

Also removed the context as you mentioned in a previous comment.

@alexcjohnson
Copy link
Collaborator

@Karan-S-Mittal the CI build error is a result of trying to build Dash from source, which has changed with Dash 2. I guess we originally did this so that the dash-daq tests would run against the tip of dash's dev branch, but I think that has outlived its usefulness and we should just install the latest released version of dash. So that would mean changing:

git clone --depth 1 https://github.com/plotly/dash.git dash-main
cd dash-main && pip install -e .[dev,testing]
cd dash-renderer && npm run build && pip install -e . && cd ../..
npm run build

To just

pip install --upgrade dash[dev,testing]

(probably don't even need the --upgrade, just included in case there's some caching I'm not seeing)

@alexcjohnson
Copy link
Collaborator

Re: extra single-quotes: Good to know that patching format_fn_name fixes it. But dash-daq only uses the one extra arg --r-prefix in its build command, there are many others which presumably suffer the same problem here:

https://github.com/plotly/dash/blob/d21a3654443bcab467a3db38a43320c08f8c7d0b/dash/development/component_generator.py#L210-L218

So I think the best solution would be to strip quotes from all of them right at that point.

Then there's still the question of how the CR characters got inserted. The easy solution would be to just delete them all at the end, but it would be better if we could figure out where they came in and prevent them in the first place.

@Karan-S-Mittal
Copy link
Contributor Author

Issues Resolved in this PR update #112, and #133

Issue #133 Resolved

issue 133

Issue #112 Resolved

issue 112

@alexcjohnson, can you review the PR now, and if possible can we merge and close the listed issues.

@jackparmer jackparmer merged commit 4975093 into plotly:master Sep 22, 2021
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.

None yet

4 participants