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

[#224] Add description for multiline puzzle #237

Merged
merged 12 commits into from
Feb 4, 2024
Merged

Conversation

pnatashap
Copy link
Contributor

@pnatashap pnatashap commented Jan 21, 2024

  1. Examples for multiline puzzles
  2. Remove unsupported examples

Fix #224

@pnatashap
Copy link
Contributor Author

@php-coder @yegor256 please take a look

@yegor256
Copy link
Member

@pnatashap looks good to me. @php-coder WDYT?

README.md Outdated Show resolved Hide resolved
@@ -172,13 +176,11 @@ as long as you have one of the 3 supported keywords right in front
of the mandatory marker):

```
// @todo #224
/* @todo #TEST-13 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not supported in the code, there is only a logic to remove prefix, but no logic to remove any kinds of symbols from the end

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I'd suggest to fix/improve support for such type of comments instead of removing it from documentation.

Copy link
Contributor Author

@pnatashap pnatashap Jan 27, 2024

Choose a reason for hiding this comment

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

There is a bug about it #225
I think it is better to remove it now and return it back after fix

Choose a reason for hiding this comment

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

@pnatashap If we remove this examples, I believe we have to remove TEST-13 and 42 from the next sentence too:

224, TEST-13, 55, 67, 678, 1, and 42 are the IDs of the tickets these puzzles are coming from.

@todo #67/DES
;; @todo #678:40m/DEV
// TODO: #1:30min
(* TODO #42 *)
Copy link
Contributor

Choose a reason for hiding this comment

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

The same: why it has been removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not supported in the code, there is only a logic to remove prefix, but no logic to remove any kinds of symbols from the end

README.md Outdated Show resolved Hide resolved
README.md Outdated
* @todo #36 Multiline text can use the same prefix in all lines or the same
* amount of spaces.
So this will be added to the puzzle description. If you want to divide the
* puzzle by empty line, just add a space to that line
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should reconsider and improve this rule. Using a trailing space for making decisions isn't good.
First, it's non-obvious.
Second, it's invisible, so it's really hard to catch such issues on code review.
Third, users may want to run auto-formatting tools, that can remove such "noise". In this case, users will face a massive close and creation of the issues.

How about using a backslash? Like this:

/**
 * @todo #36 Multiline text can use the same prefix in all lines or the same 
 *  amount of spaces.
    So this will be added to the puzzle description. If you want to divide the 
 *  puzzle by empty line, just add a backslash to that line
 *  \
 *  and continue the text after.
 *
 *  This line is not part of the puzzle, because the line before contains less
 *  spaces then the second line.
 */

@yegor256 WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@pnatashap @php-coder indeed, a "blind" space at the end of a line is provocation of a mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good symbol should be clear enough for developers.
Also will check if it works for second line as described in #163

@php-coder
Copy link
Contributor

Also, I suggest to add Fix #224 to the PR description, so the GitHub will close the related issue once the PR gets merged.

pnatashap and others added 2 commits January 22, 2024 14:07
Co-authored-by: Slava Semushin <slava.semushin@gmail.com>
Co-authored-by: Slava Semushin <slava.semushin@gmail.com>
@yegor256
Copy link
Member

@pnatashap please, make the fixed suggested and then we'll merge this one

@pnatashap
Copy link
Contributor Author

@php-coder @yegor256 please take a look

@php-coder
Copy link
Contributor

LGTM

But it breaks backward compatibility :( Ideally, we should keep it and accept both styles some time simultaneously with a warning about usage of old-style comments. But as I'm not using 0pdd anymore, so now I don't care about that much as I my repos won't have new issues created and the old one closed because of this change...

@yegor256
Copy link
Member

@volodya-lombrozo can you please take a look at this one. Do you see any problems?

@pnatashap
Copy link
Contributor Author

LGTM

But it breaks backward compatibility :( Ideally, we should keep it and accept both styles some time simultaneously with a warning about usage of old-style comments. But as I'm not using 0pdd anymore, so now I don't care about that much as I my repos won't have new issues created and the old one closed because of this change...

Actually space as a non breaking separator was a change from previous week and not released (and was not presented in documentation) so it is not about backward capability.
Removed one-line examples just not supported (I do not see any test for them)

@@ -172,13 +176,11 @@ as long as you have one of the 3 supported keywords right in front
of the mandatory marker):

```
// @todo #224
/* @todo #TEST-13 */

Choose a reason for hiding this comment

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

@pnatashap If we remove this examples, I believe we have to remove TEST-13 and 42 from the next sentence too:

224, TEST-13, 55, 67, 678, 1, and 42 are the IDs of the tickets these puzzles are coming from.


```xml
<!--
~ if comment should be started and closed by special symbols, then place them in

Choose a reason for hiding this comment

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

@pnatashap What Is "prefix" here? ~ ? I haven't found the definition of "prefix" in the README file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be used to decorate text, not required, but for there is no restrictions about the prefix, it should be just equal for all text. So it is just an example

Choose a reason for hiding this comment

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

@pnatashap, maybe we can add this to the documentation? I mean, we can state that the "prefix" is a symbol that can be used to decorate text; it isn't required. And provide some examples.

Moreover, I would suggest adding some headers or small - one-sentence descriptions to these code examples:

  1. "...Prefix definition... Here we ignore the prefix, and only one puzzle will be created."
  2. "We can also use the \ symbol to connect multiline puzzles."

What do you think?

Remove task numbers from the result description
@yegor256
Copy link
Member

yegor256 commented Feb 1, 2024

@pnatashap something wrong here with the codecov task?

@pnatashap
Copy link
Contributor Author

There is a error on report upload
[2024-01-31T17:00:02.755Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
Rerun button is not available for me, but it should work, as report created correctly

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8db7aaa) 95.50% compared to head (317b976) 95.69%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   95.50%   95.69%   +0.19%     
==========================================
  Files          10       10              
  Lines         356      372      +16     
==========================================
+ Hits          340      356      +16     
  Misses         16       16              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pnatashap
Copy link
Contributor Author

pnatashap commented Feb 1, 2024

codecov-action@v4 solves the issue with upload...
will check why it is decreased :) Check code coverage on PR creation in action

@@ -83,10 +83,6 @@
raise "STDOUT doesn't contain '#{txt}':\n#{@stdout}" unless @stdout.include?(txt)
end

Then(/^Stdout is empty$/) do

Choose a reason for hiding this comment

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

@pnatashap, I don't understand why you decided to remove some steps from steps.rb. The report states the following:

28 scenarios (23 undefined, 5 passed)
138 steps (34 skipped, 28 undefined, 76 passed)

However, we used to have:

28 scenarios (28 passed)
138 steps (138 passed)

Maybe it's worth keeping all the scenarios passing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally it was displayed, that it is duplicated steps and should be removed. Forget that they do not fail. Returned them back

Copy link
Contributor Author

@pnatashap pnatashap Feb 1, 2024

Choose a reason for hiding this comment

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

A flag is added to cucumber run to fail the task in case of undefined step


```xml
<!--
~ if comment should be started and closed by special symbols, then place them in

Choose a reason for hiding this comment

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

@pnatashap, maybe we can add this to the documentation? I mean, we can state that the "prefix" is a symbol that can be used to decorate text; it isn't required. And provide some examples.

Moreover, I would suggest adding some headers or small - one-sentence descriptions to these code examples:

  1. "...Prefix definition... Here we ignore the prefix, and only one puzzle will be created."
  2. "We can also use the \ symbol to connect multiline puzzles."

What do you think?

Copy link

@volodya-lombrozo volodya-lombrozo left a comment

Choose a reason for hiding this comment

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

@pnatashap Looks good to me. Thank you!

@pnatashap
Copy link
Contributor Author

@yegor256 please take a look

@yegor256
Copy link
Member

yegor256 commented Feb 4, 2024

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 4, 2024

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 317b976 into cqfn:master Feb 4, 2024
8 checks passed
@rultor
Copy link
Collaborator

rultor commented Feb 4, 2024

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 3min)

@pnatashap pnatashap deleted the 224 branch February 4, 2024 16:50
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.

README.md doesn't describe how pdd handles multi lines comments
5 participants