Discussion:
[mantisbt-dev] Timeline Code
P Richards
2014-06-13 21:02:04 UTC
Permalink
Following Victor's commit of the timeline API, I've pushed two follow-up fix
to my previous commit to the code base following the change to standardise
on if( as per coding standards



a) My previous pass on the code base looked for "if ( " - so missed
some cases of "if (X")

b) Victor's commit of the timeline API re-introduced code using the
wrong if style - I've just fixed these as it would generate more spam
creating 6 issues and related pull requests to track each violation.



I've just generated a Pull Request (210) following the timeline commit to
reduce the length of the index string for better compatibility with
non-mysql databases. This is at
https://github.com/mantisbt/mantisbt/pull/210. I've deliberately not pushed
this directly so we can get ensure we pick a sensible name for going
forwards.



I plan to generate a commit shortly that implements an initial pass of
phpdoc on the timeline code (I plan to just commit this to avoid creating an
issue to track each function's phpdoc, and as victor can then easily enhance
the comments as appropriate)



In terms of the feature, whilst I still think there's some work to do on it,
how do you disable it? Or is that currently impossible in its current form?



One thing I would say about merging this - github does not generate a
notification on a commit message to an existing pull request. It would be
good if in future we could add a new comment after a commit to a PR, so it's
possible to track that the status has changed of the PR.



Paul
Victor Boctor
2014-06-15 06:13:53 UTC
Permalink
Victor’s commit of the timeline API re-introduced code using the wrong if style
I did a find a replace, but it seems that I missed one of the files. In general, unless we have a style tool that runs as part of the build, we will have style issues slip by.
In terms of the feature, whilst I still think there’s some work to do on it, how do you disable it? Or is that currently impossible in its current form?
There is no way to disable it. Support disable is more work in this case, since the page layout will have to change, but the benefit was not clear to me. As for "some work to do on it", absolutely. I expect there will be enhancements that add more events, improve events, etc. But by design, I like to go incremental. Now that it is in, anyone from the team can contribute to it to add what they feel is missing rather than being just me and having features that stay outside the code for very long time with very large code reviews at the end.
One thing I would say about merging this – github does not generate a notification on a commit message to an existing pull request. It would be good if in future we could add a new comment after a commit to a PR, so it’s possible to track that the status has changed of the PR.
p***@mantisforge.org
1970-01-01 00:00:00 UTC
Permalink
From: ***@mantisforge.org
To: mantisbt-***@lists.sourceforge.net
Date: Fri, 13 Jun 2014 22:02:04 +0100
Subject: [mantisbt-dev] Timeline Code

Following Victor’s commit of the timeline API, I’ve pushed two follow-up fix to my previous commit to the code base following the change to standardise on if( as per coding standards a) My previous pass on the code base looked for “if ( “ – so missed some cases of “if (X”)b) Victor’s commit of the timeline API re-introduced code using the wrong if style – I’ve just fixed these as it would generate more spam creating 6 issues and related pull requests to track each violation. I’ve just generated a Pull Request (210) following the timeline commit to reduce the length of the index string for better compatibility with non-mysql databases. This is at https://github.com/mantisbt/mantisbt/pull/210. I’ve deliberately not pushed this directly so we can get ensure we pick a sensible name for going forwards. I plan to generate a commit shortly that implements an initial pass of phpdoc on the timeline code (I plan to just commit this to avoid creating an issue to track each function’s phpdoc, and as victor can then easily enhance the comments as appropriate) In terms of the feature, whilst I still think there’s some work to do on it, how do you disable it? Or is that currently impossible in its current form? One thing I would say about merging this – github does not generate a notification on a commit message to an existing pull request. It would be good if in future we could add a new comment after a commit to a PR, so it’s possible to track that the status has changed of the PR. Paul
------------------------------------------------------------------------------
HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions
Find What Matters Most in Your Big Data with HPCC Systems
Open Source. Fast. Scalable. Simple. Ideal for Dirty Data.
Leverages Graph Analysis for Fast Processing & Easy Data Exploration
http://p.sf.net/sfu/hpccsystems
_______________________________________________
mantisbt-dev mailing list
mantisbt-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mantisbt-dev
--_35c946a8-b17c-4f03-8dd9-89a955599c8d_
Content-Type: text/html; charset="Windows-1252"
Content-Transfer-Encoding: quoted-printable

<html>
<head>
<style><!--
.hmmessage P
{
margin:0px;
padding:0px
}
body.hmmessage
{
font-size: 12pt;
font-family:Calibri
}
--></style></head> <body class='hmmessage'><div dir='ltr'>&gt;&nbsp;<span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; text-indent: -24px; background-color: rgb(255, 255, 255);">Victor’s commit of the timeline API re-introduced code using the wrong if style&nbsp;</span><div><font color="#444444"><span style="font-size: 15px; line-height: 20.82666778564453px;"><br></span></font><div><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; text-indent: -24px; background-color: rgb(255, 255, 255);">I did a find a replace, but it seems that I missed one &nbsp;of the files. &nbsp;In general, unless we have a style tool that runs as part of the build, we will have style issues slip by.</span></div><div><br></div><div><font color="#444444"><span style="font-size: 15px; line-height: 20.82666778564453px; background-color: rgb(255, 255, 255);">&gt;&nbsp;</span></font><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; background-color: rgb(255, 255, 255);">In terms of the feature, whilst I still think there’s some work to do on it, how do you disable it? Or is that currently impossible in its current form?</span></div><div><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; background-color: rgb(255, 255, 255);"><br></span></div><div><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; background-color: rgb(255, 255, 255);">There is no way to disable it. &nbsp;Support disable is more work in this case, since the page layout will have to change, but the benefit was not clear to me. &nbsp;As for "some work to do on it", absolutely. &nbsp;I expect there will be enhancements that add more events, improve events, etc. &nbsp;But by design, I like to go incremental. &nbsp;Now that it is in, anyone from the team can contribute to it to add what they feel is missing rather than being just me and having features that stay outside the code for very long time with very large code reviews at the end.</span></div><div><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; text-indent: -24px; background-color: rgb(255, 255, 255);"><br></span></div><div><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; text-indent: -24px; background-color: rgb(255, 255, 255);">&gt;&nbsp;</span><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; background-color: rgb(255, 255, 255);">One thing I would say about merging this – github does not generate a notification on a commit message to an existing pull request. It would be good if in future we could add a new comment after a commit to a PR, so it’s possible to track that the status has changed of the PR.</span></div><div><span style="color: rgb(68, 68, 68); font-size: 15px; line-height: 20.82666778564453px; background-color: rgb(255, 255, 255);"><br></span></div><div><font color="#444444"><span style="font-size: 15px; line-height: 20.82666778564453px; background-color: rgb(255, 255, 255);">
Loading...