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

Essential fix #1 (visibility unit showing undefined) #533

Conversation

ArendPWS
Copy link
Contributor

It is really just this fix, nothing else. I really have no idea how I would be able to clarify any further.

@poblabs
Copy link
Owner

poblabs commented Jan 17, 2021

This PR shows over 145 added lines of modified code, and 121 lines removed. Is the visibility fix really this many lines?

This is showing a lot of websocket changes, graph changes, etc.

When you commit your changes, you only need to commit the few lines that are the change and not everything. Otherwise things will become out of sync. This is why I say there's conflicts and I cannot merge.

Each change should be it's own PR and not bundled. That way it's easy to track and rollback if needed in future.

Copy link
Owner

@poblabs poblabs left a comment

Choose a reason for hiding this comment

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

With the amount of changes here, it's hard for me to see the actual change that would fix the issue. That's what my "conflict" is based off of.

@ArendPWS
Copy link
Contributor Author

I have tried to explain that this is caused by the old code being included in the new code, that gives this weird looking result in the Github compare. There are multiple calls to ajaxweewx() which all need to modified. It is not logical to create a PR for each individual modified call.

@poblabs
Copy link
Owner

poblabs commented Jan 17, 2021

I have tried to explain that this is caused by the old code being included in the new code, that gives this weird looking result in the Github compare. There are multiple calls to ajaxweewx() which all need to modified. It is not logical to create a PR for each individual modified call.

I know why this is happening - you can include only the lined portions to submit to the PR - you don't have to include everything. That's what I've tried to explain 😄

@poblabs
Copy link
Owner

poblabs commented Jan 17, 2021

My suggestion - again - is to include only the lines that are changed and do not include the old code.

Why?

Because your PR would then re-introduce old code into the master branch effectively undo-ing a lot of work.

@ArendPWS
Copy link
Contributor Author

No I mean it this way:

New code {
Old code
}

That is the cause of all those apparent changes.

@ArendPWS
Copy link
Contributor Author

There is no old code in this PR

@poblabs
Copy link
Owner

poblabs commented Jan 17, 2021

Ah, it's indentation from the ajaxweewx().then(function(weewx_data) { section?

@poblabs poblabs merged commit 587e4c7 into poblabs:master Jan 17, 2021
@ArendPWS
Copy link
Contributor Author

ArendPWS commented Jan 17, 2021

YES!!!! Now we are getting on the same frequency 👍 Maybe now you have some idea why the compare looks the way it does 😃

@ArendPWS ArendPWS deleted the Essential-fix-#1-(Visibility-showing-undefined) branch January 17, 2021 20:44
michaelundwd added a commit to michaelundwd/weewx-belchertown that referenced this pull request Jan 27, 2021
corrects omission of original changes
michaelundwd added a commit to michaelundwd/weewx-belchertown that referenced this pull request Jan 27, 2021
corrects for omission when I updated to poblabs#533
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